Add --all and --repo flags to kv dump command#9874
Add --all and --repo flags to kv dump command#9874arielshaqed merged 4 commits intotreeverse:masterfrom
Conversation
- Add --all flag to dump all known partitions - Add --repo flag to dump specific repository partition - Add unit tests for dump functionality Closes treeverse#9857
|
|
arielshaqed
left a comment
There was a problem hiding this comment.
Thanks! Let's test it, and we'll go over now tests later.
| kv - kv internal metadata | ||
|
|
||
| Default: all supported sections (auth, pulls, kv)`, | ||
| Default: all supported sections (auth, pulls, kv) |
There was a problem hiding this comment.
Let's now avoid the word "all" here, given that it's but the same as the flag.
| var dump *kv.DumpFormat | ||
|
|
||
| // Handle different dump modes | ||
| if allFlag { |
There was a problem hiding this comment.
This would be easier for me to read as a switch statement.
| var repoData graveler.RepositoryData | ||
| _, err := kv.GetMsg(ctx, kvStore, graveler.RepositoriesPartition(), []byte(repoPath), &repoData) | ||
| if err != nil { | ||
| if errors.Is(err, kv.ErrNotFound) { |
There was a problem hiding this comment.
Use just errors.Is , which is false for nil. This style is used throughout lakeFS.
|
|
||
| func TestCreateDumpWithPartitions(t *testing.T) { | ||
| ctx := t.Context() | ||
| store := kvtest.GetStore(ctx, t) |
There was a problem hiding this comment.
Surprised this works tbh, cool!
| dump, err := kv.CreateDumpWithPartitions(ctx, store, []string{"partition1", "partition2"}) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, dump) | ||
| require.Len(t, dump.Entries, 3) |
There was a problem hiding this comment.
Can you verify the returned values?
- Fix duplicate graveler import - Use static error variables for linter compliance - Refactor if/else to switch statement - Simplify error handling with errors.Is pattern - Add value verification in dump tests
|
Hi @arielshaqed, thanks for taking the time to review! I've pushed a new commit addressing your feedback:
Let me know if there's anything else I should change! |
| "kv": {"kv-internal-metadata"}, // kv internal metadata | ||
| } | ||
|
|
||
| // GetAllKnownPartitions returns all partitions defined in SectionMapping |
There was a problem hiding this comment.
Why do we need this? CreateDump still tests len(partitions) == 0 and collects all known partitions.
| require.Equal(t, len(expectedPartitions), len(partitions), "should return all unique partitions") | ||
|
|
||
| for _, partition := range partitions { | ||
| require.True(t, expectedPartitions[partition], "partition %s should be in expected list", partition) | ||
| } | ||
| } |
There was a problem hiding this comment.
- Use CreateDump with empty sections for --all flag - Remove redundant GetAllKnownPartitions function and test - Fix gofmt formatting
|
Hi @arielshaqed, I've addressed your comments:
Thanks again for the helpful feedback! |
|
One last question - have you run it to see that it works (beyond unit tests, which are all well and good but...)? |
|
Yes! I've tested it locally using quickstart mode:
All flags work as expected. The dump is empty since quickstart has no data, |
|
Thanks! I should've pulled this but didn't. Will do so now. |
Summary
This PR adds two new flags to the
lakefs kv dumpcommand as requested in #9857:--all: Dumps all known partitions instead of just the predefined sections--repo <name>: Dumps only the partition for a specific repositoryChanges
--allflag to enumerate and dump all partitions from SectionMapping--repoflag to dump a specific repository's partition using gravelerGetAllKnownPartitions()andCreateDumpWithPartitions()helper functionsUsage
Test
Closes #9857