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 |
|
conn.go
Outdated
| @@ -17,6 +17,8 @@ import ( | |||
| var ( | |||
| 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? |
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.