Fix CLI tool print entire freelist. Add option to dump leaf element keys/value bytes. #50

jpbetz · 2017-09-14T06:17:54Z

This includes two small fixes to the bolt CLI that we've needed a couple times in the past--

Improve bolt page to print an accurate freelist. Won't be relevant for dbs that don't persist the freelist, but still an improvement to have the command print the most accurate information available.
Introduce bolt page-item to print a page item key and value. Had been handy for digging into data values at the bbolt level.

codecov-io · 2017-09-14T06:41:37Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2760028). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #50   +/-   ##
=========================================
  Coverage          ?   85.52%           
=========================================
  Files             ?        9           
  Lines             ?     1851           
  Branches          ?        0           
=========================================
  Hits              ?     1583           
  Misses            ?      159           
  Partials          ?      109

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2760028...69918b9. Read the comment docs.

heyitsanthony · 2017-09-14T07:26:16Z

like switching over to cobra

I don't think there should be dependencies in bbolt aside from the standard libraries since it was deliberately avoided in boltdb.

heyitsanthony

functionality looks like a candidate for a new command

also probably a good time to update .gitignore with cmd/bolt/bolt...

heyitsanthony · 2017-09-14T22:15:10Z

db_test.go

@@ -450,7 +450,7 @@ func TestDB_Open_ReadOnly(t *testing.T) {
 	// Read from a read-only transaction.
 	if err := readOnlyDB.View(func(tx *bolt.Tx) error {
 		value := tx.Bucket([]byte("widgets")).Get([]byte("foo"))
-		if bytes.Compare(value, []byte("bar")) != 0 {
+		if !bytes.Equal(value, []byte("bar")) {


already picked this up in e39821f

Great, I'll rebase.

heyitsanthony · 2017-09-14T22:24:14Z

cmd/bolt/main.go

+	if p.count == 0xFFFF {
+		idx = 1
+		count = int(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[0])
+		fmt.Fprintf(w, "Item Count of 65535 (max uint16) indicates a freelist overflow. Actual Item Count: %d\n", count)


sort of wordy/obscure, maybe only have Item Count: but determine the count depending on whether it's overflow or not? if the physical dump of the page structure is needed it should probably be handled separate from this

heyitsanthony · 2017-09-14T22:28:25Z

cmd/bolt/main.go

@@ -653,7 +714,14 @@ func (cmd *PageCommand) PrintPage(w io.Writer, r io.ReaderAt, pageID int, pageSi
 // Usage returns the help message.
 func (cmd *PageCommand) Usage() string {
 	return strings.TrimLeft(`
-usage: bolt page -page PATH pageid [pageid...]
+usage: bolt page PATH pageid [pageid...]


DumpCommand's Usage looks similarly broken, might as well fix that too...

Sure enough. Fixed.

heyitsanthony · 2017-09-14T22:34:36Z

cmd/bolt/main.go

+	p := (*page)(unsafe.Pointer(&pageBytes[0]))
+
+	if index >= p.count {
+		return nil, fmt.Errorf("Expected item index >= 0 and < %d, but got %d.", p.count, index)


fmt.Errorf("leafPageElement: expected item index less than %d, got %d", p.count, index)

to closer match the other expected errors and since index and p.count are unsigned

heyitsanthony · 2017-09-14T22:49:56Z

cmd/bolt/main.go

-		fmt.Fprintf(cmd.Stdout, "Page ID:    %d\n", p.id)
-		fmt.Fprintf(cmd.Stdout, "Page Type:  %s\n", p.Type())
-		fmt.Fprintf(cmd.Stdout, "Total Size: %d bytes\n", len(buf))
+		if options.itemKey == -1 && options.itemValue == -1 {


judging by the special-casing and that pages is described as print one or more pages in human readable format, this might be better as a separate command dump-item [-key|-value] PATH pageid idx instead of overloading the pages command

Humm, yeah. I had been thinking about splitting this out. I'll do it now.

xiang90 · 2017-09-14T23:01:32Z

@jpbetz

it seems like the bin file is also checked in. remove it?

jpbetz · 2017-09-15T05:40:53Z

Oops, I'll remove the bin file and check the .gitignore settings.

xiang90 · 2017-09-15T17:34:34Z

cmd/bolt/main.go

+		return cmd.PrintLeafItemKey(cmd.Stdout, buf, uint16(itemID))
+	case options.value:
+		return cmd.PrintLeafItemValue(cmd.Stdout, buf, uint16(itemID))
+	default:


move this to line 465, close to other validation?

xiang90 · 2017-09-15T17:34:50Z

cmd/bolt/main.go

+		return ErrUsage
+	}
+
+	if options.key && options.value {


why not allow both to be set?

That sounds reasonable. Maybe we should do something more like etcd where by default we print key and value, and provide for --key-only and --value-only flags.

right. i hope user can use the command without specifying any flag by default. right now they have to say --key or --value. If no option is provided, it will fail. not a good UX in my opinion.

xiang90 · 2017-09-15T17:35:41Z

The feature looks good in general. Let us clean up the issues mentioned in the review and get this merged. Thank you!

heyitsanthony

lgtm aside from the newline bit

heyitsanthony · 2017-09-16T00:16:54Z

cmd/bolt/main.go

+		return err
+	}
+
+	if !options.valueOnly {


if !options.valuesOnly { ... } if !options.keyOnly { ... }

so the output with no flags exactly matches the physical layout since this is a raw dump.

etcdctl's default output mode inserts a newline and in retrospect I think it makes the output really difficult / impossible to parse, but it's too late now. printf("%q\n%q\n",key,val) seems like the best/easiest human-readable way to print binary data, if that's worth pursuing in the future.

How about we do something like this:

bolt page-item <dbfile> <pageid> <itemid> "<%q encoded key>" "<%q encoded value>"

bolt page-item --raw-key-only <dbfile> <pageid> <itemid> <key bytes>

bolt page-item --raw-value-only <dbfile> <pageid> <itemid> <value bytes>

?

This solves a couple problems:

It provides a default output that won't mess up someones terminal by outputting raw bytes, and that prints out the key and value, which, if ASCII, will be rendered nicely.

It avoid use of "\n" as a delimiter between binary output.

If one needs the raw bytes, they have convenient flags they can use to get the key and value individually.

jpbetz · 2017-09-18T18:28:15Z

Based on conversation, I've moved things around a bit. Renamed the new subcommand to page-item to more clearly reflect it's purpose-- retrieve a page-item. This keeps the subcommand adjacent to the page and pages commands, which seem appropriate. (And the dump command is more for hexadecimal).

The default for page-item is to print the ascii encoded key and value in separate lines (using go's "%q" []byte formatting). This default is safe, because it doesn't dump raw bytes to a tty, which can mess up a developers terminal if they don't expect it. It also works great for basic use cases, such as printing keys and values that are plain strings.

To get to raw keys and values, switched to --raw-key-only/--raw-value-only flags, which should clearly enough hint at their purpose-- print the raw, unencoded, bytes of a key or value.

xiang90 · 2017-09-20T17:45:04Z

cmd/bolt/main.go

+	// Parse flags.
+	options := &pageItemOptions{}
+	fs := flag.NewFlagSet("", flag.ContinueOnError)
+	fs.BoolVar(&options.keyOnly, "raw-key-only", false, "Directly output the key as raw, unencoded, bytes.")


raw or encoded should be another option. i do not like the idea to fix format and output content together.

@jpbetz what do you think?

Yes, that's better, much more compose-able, and we still get our safe, easy to parse defaults. Fixing.

… of leaf elements.

xiang90 · 2017-09-20T19:13:32Z

lgtm

jpbetz force-pushed the page-kv-and-full-freelist branch from 9d9f819 to fefb4a0 Compare September 14, 2017 06:21

heyitsanthony suggested changes Sep 14, 2017

View reviewed changes

jpbetz force-pushed the page-kv-and-full-freelist branch from 8f03d2b to e4081c7 Compare September 15, 2017 06:34

xiang90 reviewed Sep 15, 2017

View reviewed changes

jpbetz force-pushed the page-kv-and-full-freelist branch from e4081c7 to ccbe64a Compare September 15, 2017 18:03

heyitsanthony reviewed Sep 16, 2017

View reviewed changes

jpbetz force-pushed the page-kv-and-full-freelist branch from ccbe64a to 6c39a12 Compare September 18, 2017 18:21

jpbetz force-pushed the page-kv-and-full-freelist branch from 6c39a12 to b02f9a7 Compare September 18, 2017 19:02

xiang90 reviewed Sep 20, 2017

View reviewed changes

Fix bolt CLI tool print entire freelist, and to dump keys/value bytes…

69918b9

… of leaf elements.

jpbetz force-pushed the page-kv-and-full-freelist branch from b02f9a7 to 69918b9 Compare September 20, 2017 19:09

xiang90 merged commit ebf39dc into etcd-io:master Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CLI tool print entire freelist. Add option to dump leaf element keys/value bytes. #50

Fix CLI tool print entire freelist. Add option to dump leaf element keys/value bytes. #50

jpbetz commented Sep 14, 2017 •

edited

Loading

codecov-io commented Sep 14, 2017 •

edited

Loading

heyitsanthony commented Sep 14, 2017

heyitsanthony left a comment

heyitsanthony Sep 14, 2017

jpbetz Sep 15, 2017

heyitsanthony Sep 14, 2017

heyitsanthony Sep 14, 2017

jpbetz Sep 15, 2017

heyitsanthony Sep 14, 2017

heyitsanthony Sep 14, 2017

jpbetz Sep 15, 2017

xiang90 commented Sep 14, 2017

jpbetz commented Sep 15, 2017

xiang90 Sep 15, 2017

xiang90 Sep 15, 2017

jpbetz Sep 15, 2017

xiang90 Sep 15, 2017

xiang90 commented Sep 15, 2017

heyitsanthony left a comment

heyitsanthony Sep 16, 2017 •

edited

Loading

jpbetz Sep 18, 2017 •

edited

Loading

jpbetz commented Sep 18, 2017 •

edited

Loading

xiang90 Sep 20, 2017

jpbetz Sep 20, 2017

xiang90 commented Sep 20, 2017

Fix CLI tool print entire freelist. Add option to dump leaf element keys/value bytes. #50

Fix CLI tool print entire freelist. Add option to dump leaf element keys/value bytes. #50

Conversation

jpbetz commented Sep 14, 2017 • edited Loading

codecov-io commented Sep 14, 2017 • edited Loading

Codecov Report

heyitsanthony commented Sep 14, 2017

heyitsanthony left a 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

xiang90 commented Sep 14, 2017

jpbetz commented Sep 15, 2017

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

xiang90 commented Sep 15, 2017

heyitsanthony left a comment

Choose a reason for hiding this comment

heyitsanthony Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

jpbetz Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

jpbetz commented Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Sep 20, 2017

jpbetz commented Sep 14, 2017 •

edited

Loading

codecov-io commented Sep 14, 2017 •

edited

Loading

heyitsanthony Sep 16, 2017 •

edited

Loading

jpbetz Sep 18, 2017 •

edited

Loading

jpbetz commented Sep 18, 2017 •

edited

Loading