- Notifications
You must be signed in to change notification settings - Fork1.1k
fix: reinstate the appropriate transaction for prebuild reconciliation#20587
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
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.
LGTM 🔑
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.
I'm OK with this as a quick fix, but I might suggest going further and replacingc.store entirely withInTx. Could be a separate refactor though.
I just saw a comment:
I need to investigate this before we merge this PR. |
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 internally inSlack: After thinking this through, I realized the current transaction design is actually correct. We shouldn't change the outer transaction to read-write or passtx to the membership reconciler.
The outer transaction should remain read-only: its purpose is locking and consistent snapshotting. Write operations (membership inserts, goroutine creates/deletes) should continue usingc.store to create separate transactions, allowing successful writes to commit independently even if reconciliation fails later.
What we should change in this PR:
- Update read operations within a transaction to use
tx - Keep write operations using
c.store
Uh oh!
There was an error while loading.Please reload this page.
This pull request fixes a bug that was introduced in#18010, which resulted in the main reconciliation transaction being unused. Instead, all database related methods were accessing c.store directly.
The next step will be to replace usages of c.store within the reconciliation loop with an injected transaction parameter to represent the transaction to use.