-
Notifications
You must be signed in to change notification settings - Fork 23
[OCTRL-985] Place kafka messages related to same environments in the same partition #655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
common/event/writer.go
Outdated
| case *pb.Ev_RunEvent: | ||
| // we are using RunNumber as a key here, requested by M.Boulais | ||
| key = make([]byte, 4) | ||
| binary.BigEndian.PutUint32(key, e.RunNumber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do i correctly understand that the consequence of this is that we will not have synchronization between RunEvents and all the others, as they use different keys? What is the advantage of using run number here, if env ID should be anyway available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is the reason for the comment. Martin wanted to use run number for this event in ticket, so I used it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the way, RunEvents are in their own topic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed in person, please cross-check with Martin what is the reason for this exception just to be sure, but I'm OK with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in person: Martin is on vacation this week, I sent him a message. I will report when he is back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not releasing this week, so it's anyway fine to wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Martin agreed that using envId is fine here. So I did amend previous commit and reworded it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
- added environment id as a key to messages containing environment id field - changed Balancer from LeastBytes to Hash which makes kafka-go puts messages with same key to the same partition, fixing ordering problems
I did not add any tests, because I was unsure how to test this properly, so I just tested this on my openstack machine.