Skip to content
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

[3.5] Backport fix tx buffer inconsistency if there are unordered key writes in one tx #18861

Open
wants to merge 1 commit into
base: release-3.5
Choose a base branch
from

Conversation

chaochn47
Copy link
Member

Fix tx buffer inconsistency if there are unordered key writes in one tx.

This bug fix seems like an important one we need to get into release branch in #18846.

WDTY? @ahrtr @serathius @fuweid @ivanvc

Notes:

  1. batch_tx_test.go is not incorporated into this backport because main and release-3.5 diverges a lot on buckets/schema/etc.. It is also not the core of the bug fix.
  2. verify in tx_buffer.go is removed as mainly due to the dependency is not there in relese-3.5.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@ivanvc ivanvc mentioned this pull request Nov 10, 2024
2 tasks
@chaochn47
Copy link
Member Author

/retest

@chaochn47
Copy link
Member Author

Hi @wenjiaswe, as the bot suggested, could you please take a look at the backport PR?

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Could you please also backport the fix to 3.4 and update both 3.5 and 3.4 changelog?

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, chaochn47

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR comes with enough tests to feel safe. Testing just dedup function is not enough to ensure there is no regression with the backport. Code differences should make us even more careful and ensure the change is tested instead of removing tests if it's not convenient.

Correctly me if I'm wrong but If I remember correctly, the fix isn't for a user visible bug, as the issue is internal and requires other changes that were done on main branch to be visible. This means we should not rush this backport.

@jmhbnz
Copy link
Member

jmhbnz commented Nov 11, 2024

/retitle [3.5] Backport fix tx buffer inconsistency if there are unordered key writes in one tx

@k8s-ci-robot k8s-ci-robot changed the title release-3.5: backport #17263 [3.5] Backport fix tx buffer inconsistency if there are unordered key writes in one tx Nov 11, 2024
…ere are unordered key writes in one tx.

etcd-io#17263

Notes:
1. batch_tx_test.go is not incorporated into this backport because main and release-3.5 diverges a lot on buckets/schema/etc..
It is also not the core of the bug fix.
2. verify in tx_buffer.go is removed as mainly due to the dependency is not there in relese-3.5.

Signed-off-by: Chao Chen <chaochn@amazon.com>
@chaochn47
Copy link
Member Author

Testing just dedup function is not enough to ensure there is no regression with the backport. Code differences should make us even more careful and ensure the change is tested instead of removing tests if it's not convenient.

Thanks for catching this. Just realized replacing schema with buckets should be able to backport the test coverage of this change in batch_tx_test.go.

as the issue is internal and requires other changes that were done on main branch to be visible.

Hi @serathius could you please elaborate what the other change is in main branch that was disclosing this bug?

Is it because another NEW key value was written into the non-key buckets without proper lexicographical order?

This means we should not rush this backport.

Agreed. Safety of correctness is the key for release branches. Could you please suggest what additional safeguard / testing coverage we should have before committing this back port?

@chaochn47
Copy link
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants