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

Add internal iterator to Bucket that goes over buckets. #356

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Dec 17, 2022

So far the code was frequently traversing all the keys (ignoring flag whether key is a bucket) and trying to open each of the keys as bucket (seeking the same entry from the scratch).

In this proposal, we iterate only through bucket keys.

Signed-off-by: Piotr Tabor ptab@google.com

cursor.go Outdated
return k, v
}

func (c *Cursor) first() (key []byte, value []byte, flags uint32) {
_assert(c.bucket.tx.db != nil, "tx closed")
Copy link
Member

Choose a reason for hiding this comment

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

Suggest to keep the existing check behavior: only check the _assert(c.bucket.tx.db != nil, "tx closed") on the exported Methods (e.g. First, Last, Next, Prev).

But seek is an abnormal example. We may need to move the check to Seek.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

bucket_test.go Outdated
}
err := db.Update(func(tx *bolt.Tx) error {
b, err := tx.CreateBucket([]byte("widgets"))
assert.NoError(t, err, "bucket creation failed")
Copy link
Member

Choose a reason for hiding this comment

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

You might need to use require.NoError. Note assert.NoError will not terminate the execution of the case if it fails. Obviously it doesn't make sense to continue to execute if creaing bucket fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. It's misleading in testify that assertions are not an assertions.

Done.

@ahrtr
Copy link
Member

ahrtr commented Dec 21, 2022

Overall looks good to me. Please resolve the conflict.

So far the code was frequently traversing all the keys (ignoring flag whether key is a bucket)
and trying to open each of the keys as bucket (seeking the same entry from the scratch).

In this proposal, we iterate only through bucket keys.

Signed-off-by: Piotr Tabor <ptab@google.com>
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

@ptabor
Copy link
Contributor Author

ptabor commented Dec 21, 2022

Thank you for review.

@ptabor ptabor merged commit 696c85c into etcd-io:master Dec 21, 2022
@ahrtr ahrtr added this to the 1.3.7 milestone Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants