- Notifications
You must be signed in to change notification settings - Fork837
Do not commit offsets for past generations if partition not owned#1330
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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 |
|
89f3edd to49e263bCompareAtlantis 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.
49e263b to8091163CompareAtlantis 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
| errInvalidWriteTopic=errors.New("writes must NOT set Topic on kafka.Message") | ||
| errInvalidWritePartition=errors.New("writes must NOT set Partition on kafka.Message") | ||
| undefinedGenerationIdint32=-1 |
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.
It should beconst, notvar.
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.
thanks for the feedback, I applied the suggestion, I would also appreciate feedback on the general approach as well 🙏
logrusorgru commentedDec 20, 2024
@nachogiljaldo , please, if it’s not a problem for you, apply this patchnachogiljaldo#1 . (No changes in logic). |
logrusorgru commentedDec 20, 2024
Interesting, it means maintainers review? Or it can accept my review too? |
…_generations_if_partition_not_ownedRename *Id -> *ID, following Go naming convention.
Thanks@logrusorgru
I believe a maintainer :( |
M0rdecay commentedFeb 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hello! Any chances new release will be published with this patch? UPD 27.02 |
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.