Do not commit offsets for past generations if partition not owned#1330
Conversation
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
nachogiljaldo
force-pushed
the
do_not_commit_offsets_for_past_generations_if_partition_not_owned
branch
from
89f3edd to
49e263b
Compare
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
…at are not valid anymore.
nachogiljaldo
force-pushed
the
do_not_commit_offsets_for_past_generations_if_partition_not_owned
branch
from
49e263b to
8091163
Compare
|
Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors |
|
| errInvalidWriteTopic = errors.New("writes must NOT set Topic on kafka.Message") | ||
| errInvalidWritePartition = errors.New("writes must NOT set Partition on kafka.Message") | ||
|
|
||
| undefinedGenerationId int32 = -1 |
There was a problem hiding this comment.
thanks for the feedback, I applied the suggestion, I would also appreciate feedback on the general approach as well 🙏
|
@nachogiljaldo , please, if it’s not a problem for you, apply this patch nachogiljaldo#1 . (No changes in logic). |
Interesting, it means maintainers review? Or it can accept my review too? |
…_generations_if_partition_not_owned Rename *Id -> *ID, following Go naming convention.
Thanks @logrusorgru
I believe a maintainer :( |
|
Hello! Any chances new release will be published with this patch? UPD 27.02 |
3 participants
This is a skeleton PR aiming for discussion related to #1308
In that issue, I describe how rebalances can lead to a situation where the consumer receiving the partition does not read messages in spite of lag existing (because conn's offset is > than the broker's current offset and there's no new messages coming).
I was aiming to fix it by avoiding commits for past generations, to do so, I add the generation id to the message and when committing, I log an error instead of adding it to the stash if it belongs to a generation < than current.
This has the risk of losing valid commits when the new generation comes (as its associated generation < latest generation.id), however it works because a new generation ends up creating new connections which means uncommitted events are duplicated. It is not that I love it because it works incidentally but it does work as my test shows.
If you have a different / better approach I am happy to explore it if you do explain it a bit.