fix: change pageItemCommand default format auto #596

Elbehery · 2023-11-06T18:21:06Z

This PR changes pageItemCommand default format from ascii-encoded to auto

resolves ##588

ahrtr · 2023-11-07T13:02:37Z

cmd/bbolt/main_test.go

+			func(tx int, k int) []byte { return convertInt64IntoBytes(t, 10001) },
+			func(tx int, k int) []byte { return convertInt64IntoBytes(t, 10002) },


Suggested change

func(tx int, k int) []byte { return convertInt64IntoBytes(t, 10001) },

func(tx int, k int) []byte { return convertInt64IntoBytes(t, 10002) },

func(tx int, k int) []byte { return convertInt64IntoBytes(t, k+1) },

func(tx int, k int) []byte { return convertInt64IntoBytes(t, k+2) },

ahrtr · 2023-11-07T13:03:58Z

cmd/bbolt/main_test.go

+		m := NewMain()
+		err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), "0")
+		require.NoError(t, err)
+		if !strings.Contains(m.Stdout.String(), "10001") || !strings.Contains(m.Stdout.String(), "10002") {


This isn't correct. When the key or value isn't printable, the output will be hex format string.

ahrtr · 2023-11-07T13:04:34Z

For your reference:

func TestPageItemCommand_Run(t *testing.T) {
	testCases := []struct {
		name          string
		printable     bool
		itemID        string
		expectedKey   string
		expectedValue string
	}{
		{
			name:          "printable items",
			printable:     true,
			itemID:        "0",
			expectedKey:   "key_0",
			expectedValue: "value_0",
		},
		{
			name:          "non-printable items",
			printable:     false,
			itemID:        "0",
			expectedKey:   hex.EncodeToString(convertInt64ToBytes(0 + 1)),
			expectedValue: hex.EncodeToString(convertInt64ToBytes(0 + 2)),
		},
	}

	for _, tc := range testCases {
		tc := tc
		t.Run(tc.name, func(t *testing.T) {

			db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: 4096})
			srcPath := db.Path()

			// Insert some sample data
			t.Log("Insert some sample data")
			err := db.Update(func(tx *bolt.Tx) error {
				b, berr := tx.CreateBucketIfNotExists([]byte("data"))
				if berr != nil {
					return berr
				}
				for i := 0; i < 100; i++ {
					// write printable key/value
					if tc.printable {
						if perr := b.Put([]byte(fmt.Sprintf("key_%d", i)), []byte(fmt.Sprintf("value_%d", i))); perr != nil {
							return perr
						}
					} else {
						k, v := convertInt64ToBytes(int64(i+1)), convertInt64ToBytes(int64(i+2))
						if perr := b.Put(k, v); perr != nil {
							return perr
						}
					}
				}
				return nil
			})
			require.NoError(t, err)

			defer requireDBNoChange(t, dbData(t, srcPath), srcPath)

			meta := readMetaPage(t, srcPath)
			leafPageId := 0
			for i := 2; i < int(meta.Pgid()); i++ {
				p, _, err := guts_cli.ReadPage(srcPath, uint64(i))
				require.NoError(t, err)
				if p.IsLeafPage() && p.Count() > 1 {
					leafPageId = int(p.Id())
				}
			}
			require.NotEqual(t, 0, leafPageId)

			m := NewMain()
			err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), tc.itemID)
			require.NoError(t, err)
			if !strings.Contains(m.Stdout.String(), tc.expectedKey) || !strings.Contains(m.Stdout.String(), tc.expectedValue) {
				t.Fatalf("Unexpected output:\n%s\n", m.Stdout.String())
			}
		})
	}
}

func convertInt64ToBytes(num int64) []byte {
	buf := make([]byte, binary.MaxVarintLen64)
	n := binary.PutVarint(buf, num)
	return buf[:n]
}

Elbehery · 2023-11-07T13:58:23Z

@ahrtr fixed 👍🏽

Elbehery · 2023-11-07T13:58:56Z

cmd/bbolt/main_test.go

+		{
+			name:          "non printable items",
+			printable:     false,
+			itemId:        "0",


Q : why we use the same itemId as the printable item above ?

Yes, good question. You can even check all items using a for loop,

for i:=0; i<100; i++ { // check $i item }

but we are using only one itemId for all the data, no ?

Elbehery · 2023-11-07T13:59:49Z

cmd/bbolt/main_test.go

+			name:          "non printable items",
+			printable:     false,
+			itemId:        "0",
+			expectedKey:   hex.EncodeToString(convertInt64IntoBytes(0 + 1)),


Suggested change

expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),

expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),

Why we use key as 0+1 and value as 0+2 ?

Elbehery · 2023-11-07T14:00:31Z

cmd/bbolt/main_test.go

+							return bErr
+						}
+					} else {
+						k, v := convertInt64IntoBytes(int64(i+1)), convertInt64IntoBytes(int64(i+2))


same here, why we increment the i ? .. not the case in the printable data above !

Elbehery · 2023-11-07T14:01:22Z

cmd/bbolt/main_test.go

+				return nil
+			})
+			require.NoError(t, err)
+			defer requireDBNoChange(t, dbData(t, srcPath), srcPath)


i wonder, why the database should not change, since we have inserted sample data ?

Note that purpose of the requireDBNoChange is to ensure the command doesn't change the db's data.

so dbData read the database file that is already on-disk .. then requireDBNoChange will read the new data file after the test data has been inserted in the transaction above ..

My question is, how the database should not change, while we are inserting sample data ?

please correct me if i am wrong

When a "defer" statement gets executed, its value and parameters to the call are evaluated as usual and saved anew, so dbData(t, srcPath) is executed right away. It reads current db's data. Note that the we have already finished inserting all data at the moment.

But the actual function is not invoked until the surrounding function returns. When it gets really executed, it reads the db's data again, and compare it with the previous data read in dbData(t, srcPath).

The rough workflow:

insert some sample data

read the db's data using dbData(t, srcPath)

execute the command (the main part of the test);

read the db's data again, and compare it with the data read at step 2.

gotcha, makes perfect sense now :) .. thanks tonsss 🙏🏽

ahrtr · 2023-11-07T14:09:46Z

cmd/bbolt/main_test.go

+			m := NewMain()
+			err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), tc.itemId)
+			require.NoError(t, err)
+			if !strings.Contains(m.Stdout.String(), tc.expectedKey) || !strings.Contains(m.Stdout.String(), tc.expectedValue) {
+				t.Fatalf("Unexpected output:\n%s\n", m.Stdout.String())
+			}


If you want, you can also enhance the test case something like below,

for i:=0; i<100; i++ { m := NewMain() err = m.Run("page-item", db.Path(), fmt.Sprintf("%d", leafPageId), fmt.Sprintf("%d", i)) require.NoError(t, err) if !strings.Contains(m.Stdout.String(), xxx) || !strings.Contains(m.Stdout.String(), yyy) { t.Fatalf("Unexpected output:\n%s\n", m.Stdout.String()) } }

Then you can remove

itemId string expectedKey string expectedValue string

i like the anonymous struct style more, because it is easier to be read by others.. thanks for your answer 🙏🏽

ahrtr · 2023-11-07T14:13:58Z

I assume #596 (comment) answered all your questions.

ahrtr · 2023-11-07T14:15:26Z

cmd/bbolt/main_test.go

 	"fmt"
+	"go.etcd.io/bbolt/internal/guts_cli"


Need to put all go.etcd.io/xxx together.

done 👍🏽

Signed-off-by: Mustafa Elbehery <[email protected]>

ahrtr · 2023-11-07T15:12:41Z

One more minor comment, the readme doc https://github.com/etcd-io/bbolt/tree/master/cmd/bbolt#page-item also needs to be updated. The default format changed. This can be addressed in this PR or a followup PR.

Elbehery · 2023-11-07T15:20:10Z

One more minor comment, the readme doc https://github.com/etcd-io/bbolt/tree/master/cmd/bbolt#page-item also needs to be updated. The default format changed. This can be addressed in this PR or a followup PR.

thanks for your comment .. I propose to change the Readme once in a follow up PR, because the CI is almost done on this PR .. wdyt ?

ahrtr · 2023-11-07T15:21:11Z

I propose to change the Readme once in a follow up PR, because the CI is almost done on this PR .. wdyt ?

OK.

ahrtr

LGTM

Congratulations on your first PR! Thanks.

Elbehery force-pushed the pageItemCommand_default_format_auto branch from f42d8cb to f086276 Compare November 6, 2023 18:22

This was referenced Nov 6, 2023

fix:change keysCommand default format auto #597

Merged

fix bucket keys as ints format #590

Closed

ahrtr reviewed Nov 7, 2023

View reviewed changes

Elbehery force-pushed the pageItemCommand_default_format_auto branch from f086276 to 14448c3 Compare November 7, 2023 13:58

Elbehery commented Nov 7, 2023

View reviewed changes

ahrtr reviewed Nov 7, 2023

View reviewed changes

fix: change pageItemCommand default format auto

f56cd26

Signed-off-by: Mustafa Elbehery <[email protected]>

Elbehery force-pushed the pageItemCommand_default_format_auto branch from 14448c3 to f56cd26 Compare November 7, 2023 14:21

ahrtr approved these changes Nov 7, 2023

View reviewed changes

ahrtr merged commit 959ab0a into etcd-io:master Nov 7, 2023
9 checks passed

Elbehery deleted the pageItemCommand_default_format_auto branch November 7, 2023 16:04

Elbehery mentioned this pull request Feb 13, 2024

Cleanup Kubernetes GitHub organizations - Jan 2024 kubernetes/org#4726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change pageItemCommand default format auto #596

fix: change pageItemCommand default format auto #596

Elbehery commented Nov 6, 2023

ahrtr Nov 7, 2023

ahrtr Nov 7, 2023

ahrtr commented Nov 7, 2023

Elbehery commented Nov 7, 2023

Elbehery Nov 7, 2023

ahrtr Nov 7, 2023

Elbehery Nov 7, 2023

Elbehery Nov 7, 2023

Elbehery Nov 7, 2023

Elbehery Nov 7, 2023

ahrtr Nov 7, 2023

Elbehery Nov 7, 2023

ahrtr Nov 7, 2023

ahrtr Nov 7, 2023

Elbehery Nov 7, 2023

ahrtr Nov 7, 2023 •

edited

Loading

Elbehery Nov 7, 2023

ahrtr commented Nov 7, 2023

ahrtr Nov 7, 2023

Elbehery Nov 7, 2023

ahrtr commented Nov 7, 2023

Elbehery commented Nov 7, 2023

ahrtr commented Nov 7, 2023

ahrtr left a comment

		func(tx int, k int) []byte { return convertInt64IntoBytes(t, 10001) },
		func(tx int, k int) []byte { return convertInt64IntoBytes(t, 10002) },

	expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),
	expectedKey: hex.EncodeToString(convertInt64IntoBytes(0 + 1)),

fix: change pageItemCommand default format auto #596

fix: change pageItemCommand default format auto #596

Conversation

Elbehery commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Nov 7, 2023

Elbehery commented Nov 7, 2023

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

Choose a reason for hiding this comment

ahrtr Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Nov 7, 2023

Elbehery commented Nov 7, 2023

ahrtr commented Nov 7, 2023

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr Nov 7, 2023 •

edited

Loading