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

backend: set seq flag for each bucket buffer #12568

Closed
wants to merge 1 commit into from

Conversation

mlmhl
Copy link
Contributor

@mlmhl mlmhl commented Dec 17, 2020

This PR solves two problems:

  1. Reset the seq flag to true when resetting the whole txWriteBuffer, otherwise the writeback method always sort the buffer once seq set to false
  2. Give each bucket buffer a seq flag to reduce sort times, as most of the data is stored in the key bucket, which grows in sequence.

@ptabor ptabor self-assigned this Jan 31, 2021
Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Nice. Thank you.

Please retrigger tests and I will merge when green.

@@ -37,11 +37,12 @@ func (txb *txBuffer) reset() {
// txWriteBuffer buffers writes of pending updates that have not yet committed.
type txWriteBuffer struct {
txBuffer
seq bool
seq map[string]bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment about semantic of this field (what's the key and what the value represents).

@ptabor
Copy link
Contributor

ptabor commented Feb 5, 2021

./scripts/fix.sh should fix the formatting problem (superfluous empty line).

@gyuho
Copy link
Contributor

gyuho commented Feb 23, 2021

@mlmhl Could you address @ptabor's comment?

@ptabor ptabor added this to the etcd-v3.5 milestone Mar 1, 2021
ptabor added a commit that referenced this pull request May 14, 2021
ptabor added a commit that referenced this pull request May 14, 2021
@ptabor
Copy link
Contributor

ptabor commented May 14, 2021

I applied my suggestions on top of the PR - so from my perspective its good for merge.

@wilsonwang371 - could you, please, take a look as its related to code you are actively working on.

@ptabor
Copy link
Contributor

ptabor commented May 15, 2021

The branch tests a green:
c278070

The PR was pushed before the reorg - so the tests results are not shown on the PR-level

Copy link
Contributor

@wilsonwang371 wilsonwang371 left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@wilsonwang371
Copy link
Contributor

Btw, shall we change the variable name to make it more clear with the new changes?

@ptabor
Copy link
Contributor

ptabor commented May 17, 2021

Migrated to: #12986

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.

4 participants