Move sub bucket #625

Elbehery · 2023-11-27T17:45:55Z

…nother bucket Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

Elbehery · 2023-11-27T17:47:45Z

bucket_test.go

+				if v == nil {
+					t.Fatal()
+				}
+				//curDst := dstBucket.Cursor()


I tried to use seek() to verify the subBucket is moved correctly, but I got stuck :/

I still need to

check for cornet cases

verify that

// 1. the sub-bucket cannot be found in the source bucket; // 2. or the key already exists in the destination bucket; // 3. the key represents a non-bucket value.

ptal if i am on the right path, otherwise correct me please ( fail fast ) :D

cc @ahrtr ^^

Elbehery · 2023-11-28T14:07:14Z

@ahrtr i finished the test, i covered all the possible cases iiuc :)

ptal

cc @tjungblu @fuweid @serathius

fuweid · 2023-11-29T03:09:42Z

tx.go

+//  1. the sub-bucket cannot be found in the source bucket;
+//  2. or the key already exists in the destination bucket;
+//  3. the key represents a non-bucket value.
+func (tx *Tx) MoveSubBucket(name []byte, src *Bucket, dst *Bucket) error {


It seems tx is unused here. I think we don't need this method.

good catch!

I was thinking to support moving top level bucket. In that case, no need to provide the source bucket, the signature & implementation will be below,

func (tx *Tx) MoveSubBucket(name []byte, src *Bucket, dst *Bucket) error { if src != nil { return src.MoveSubBucket(name, dst) } return tx.root.MoveSubBucket(name, dst) }

@Elbehery Could you help to update it in this PR?

@ahrtr I am confused now

how can I specify the root bucket explicitly, it is being init by the tx

bbolt/tx.go

Line 54 in 4261e22

// Copy over the root bucket.

Yes. tx.root is root bucket.

fuweid · 2023-11-29T04:02:34Z

bucket_test.go

+		name                    string
+		srcBucketName           string
+		subBucketName           string
+		subBucketKey            string


Maybe we can add a case to create non-inline sub bucket here.

u mean to use the src bucket as root bucket ?

For the bucket, if the key or sub-bucket is small, the bucket can store those in value. It' s inline type.
If the key/value is large enough to allocate new page(s) for that, the bucket value only stores the root page id.
Just want to verify these two cases.

Yeah, since you change the tx.MoveSubBucket to support root bucket, it should add some cases to cover that.

Elbehery · 2023-11-29T11:21:51Z

bucket_test.go

+					t.Fatalf("dst bucket %s does not exist: %v", tc.dstBucketName, berrors.ErrBucketNotFound)
+				}
+
+				mvErr := tx.MoveSubBucket([]byte(tc.subBucketName), srcBucket, dstBucket)


@ahrtr @fuweid i have used it here

I think it is better to keep this method, so we can create move-bucket cmd later on, wdyt ?

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

Elbehery · 2023-11-30T18:45:05Z

tx_test.go

+			nil,
+		},
+		{
+			"subBucket not exist in root bucket",


@fuweid @ahrtr I added these tests to tx_text, i adapted the code to test only the case when the srcBucket is nil, since these cases are covered by bucket_test

ptal

The dst could be nil as well. Please take a look at the original PR.

fuweid · 2023-12-01T03:06:25Z

bucket_test.go

+		subBucketExistDstBucket bool
+		keyNoSubBucketSrcBucket bool
+		keyNoSubBucketDstBucket bool
+		expErr                  error


Overall, it looks good to me.

For the test case, I was thinking about that introducing slice to define the path, since it's hard to follow the code with too many bools here.

During preparing data, that test case can use srcBucketPath []string to define depth of bucket. For instance,
[x, y, z]. The bucket will be like root -> x -> y -> z. srcKeyValues [][2]string is to define key value in the srcBucketPath. If it's empty, That z doesn't contain any key values. It also applies to dst.

If there is no expected error, the root -> x -> y -> z should be not found and that z should show in dstBucketPath. And all the srcKeyValues should be stored in dstBucketPath -> z.

WDYT?

i can change it, but i think the booleans are easier for a reader to understand the what & how

maybe the bool names needs to be more self explanatory

wdyt @ahrtr @serathius @tjungblu

ahrtr · 2023-12-03T14:19:48Z

I updated the original PR #621, Please take a look.

ahrtr · 2023-12-04T13:53:31Z

High level the test workflow including three steps make sense,

create sample database
move a child bucket
verify there is no any change for child bucket's content before and after the moving

Comments:

The sample database should contain multiple possibilities
- Contain big key/value so that the overflow in page header > 1
- Huge number of key/value so that it can't be nested bucket
- The src or dst bucket are the root bucket
- There are multiple layer of the child bucket.
- negative cases
Make the verification more generic
- Dump the child bucket into a separate [bbolt db] file before moving it;
- Dump the child bucket into another separate [bbolt db] file after the moving;
- Compare the two files.

ahrtr · 2023-12-12T10:51:22Z

Sorry, the PR is automatically closed after renaming the master to main. Please feel free to submit a new PR.

add MoveSubBucket to support moving a sub-bucket from one bucket to a…

4261e22

…nother bucket Signed-off-by: Benjamin Wang <benjamin.ahrtr@gmail.com>

Elbehery commented Nov 27, 2023

View reviewed changes

Elbehery force-pushed the move_subBucket branch 4 times, most recently from 381b51e to accffe2 Compare November 28, 2023 14:05

fuweid reviewed Nov 29, 2023

View reviewed changes

Elbehery commented Nov 29, 2023

View reviewed changes

Elbehery force-pushed the move_subBucket branch from accffe2 to 51e0df3 Compare November 29, 2023 15:07

add MoveSubBucket test

148da9e

Signed-off-by: Mustafa Elbehery <melbeher@redhat.com>

Elbehery force-pushed the move_subBucket branch from 51e0df3 to 148da9e Compare November 30, 2023 18:43

Elbehery commented Nov 30, 2023

View reviewed changes

fuweid reviewed Dec 1, 2023

View reviewed changes

ahrtr deleted the branch etcd-io:master December 12, 2023 10:26

ahrtr closed this Dec 12, 2023

Elbehery mentioned this pull request Dec 12, 2023

add MoveBucket to support moving a sub-bucket from one bucket to anot… #635

Merged

Elbehery deleted the move_subBucket branch January 12, 2024 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move sub bucket #625

Move sub bucket #625

Elbehery commented Nov 27, 2023

Elbehery Nov 27, 2023

Elbehery Nov 27, 2023

Elbehery commented Nov 28, 2023

fuweid Nov 29, 2023 •

edited

Loading

ahrtr Nov 29, 2023 •

edited

Loading

ahrtr Nov 29, 2023

Elbehery Nov 29, 2023

fuweid Nov 30, 2023

fuweid Nov 29, 2023

Elbehery Nov 29, 2023

fuweid Nov 30, 2023

Elbehery Nov 29, 2023

Elbehery Nov 30, 2023

ahrtr Dec 3, 2023

fuweid Dec 1, 2023 •

edited

Loading

Elbehery Dec 1, 2023

ahrtr commented Dec 3, 2023

ahrtr commented Dec 4, 2023

ahrtr commented Dec 12, 2023

Move sub bucket #625

Move sub bucket #625

Conversation

Elbehery commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elbehery commented Nov 28, 2023

fuweid Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

ahrtr Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Dec 3, 2023

ahrtr commented Dec 4, 2023

ahrtr commented Dec 12, 2023

fuweid Nov 29, 2023 •

edited

Loading

ahrtr Nov 29, 2023 •

edited

Loading

fuweid Dec 1, 2023 •

edited

Loading