cli: Add etcdutl snapshot hashkv command #15965

cenkalti · 2023-05-26T19:54:04Z

% bin/etcdctl endpoint hashkv -w json | jq
[
  {
    "Endpoint": "127.0.0.1:2379",
    "HashKV": {
      "header": {
        "cluster_id": 14841639068965180000,
        "member_id": 10276657743932975000,
        "revision": 214,
        "raft_term": 17
      },
      "hash": 902327963,
      "compact_revision": 150,
      "hash_revision": 214
    }
  }
]
% bin/etcdutl snapshot hashkv default.etcd/member/snap/db -w json | jq
{
  "hash": 902327963,
  "hashRevision": 214,
  "compactRevision": 150
}

jmhbnz

Nice work @cenkalti.

Should we also add a quick segment of documentation to the README.md at https://github.com/etcd-io/etcd/tree/main/etcdutl?

cenkalti · 2023-05-26T21:07:40Z

@jmhbnz Added.

jmhbnz

LGTM - Thanks @cenkalti.

Note at some point I think we need to take some of the README.md content and put together some overall human friendly documentation for etcdutl on the website. Something similar to https://etcd.io/docs/v3.5/dev-guide/interacting_v3. Perhaps named Interacting with etcd data files. A separate issue than this pr though.

etcdutl/snapshot/v3_snapshot.go

etcdutl/etcdutl/printer.go

etcdutl/etcdutl/snapshot_command.go

ahrtr

Please resolve the review comments.

ahrtr · 2023-06-02T08:19:57Z

etcdutl/etcdutl/snapshot_command.go

@@ -48,6 +49,7 @@ func NewSnapshotCommand() *cobra.Command {
 	}
 	cmd.AddCommand(NewSnapshotRestoreCommand())
 	cmd.AddCommand(newSnapshotStatusCommand())
+	cmd.AddCommand(newSnapshotHashKVCommand())


Probably it makes more sense to add a separate command something like etcdutl hashkv <filename> [flags], because it isn't necessary related to a snapshot.

stale · 2023-09-17T01:16:36Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

cenkalti · 2024-01-19T19:47:12Z

/retest

jmhbnz · 2024-03-28T18:05:57Z

Hey @cenkalti - We highlighted this older pr during the sig-etcd triage meeting today. It would be great to get this tidied up and merged given we already had partial maintainer approval.

Can you please rebase and address the remaining comments? Thanks! 🙏🏻

cenkalti · 2024-03-28T18:27:28Z

I'll do it this weekend.

cenkalti · 2024-03-30T15:09:26Z

Rebased. Ready to merge.

ahrtr · 2024-05-23T18:35:10Z

etcdutl/etcdutl/hashkv_command.go

+	if len(args) != 1 {
+		err := fmt.Errorf("hashkv requires exactly one argument")
+		cobrautl.ExitWithError(cobrautl.ExitBadArgs, err)


Suggest to use cobra.ExactArgs(1), but not a blocker to merge this PR.

Refer to https://github.com/etcd-io/bbolt/blob/f663d4e6043e37fa77c3b554f2692d4d02cc8c95/cmd/bbolt/command_surgery_freelist.go#L31

ahrtr · 2024-05-23T18:39:15Z

We also need to add a changelog item, but it's OK to add it in a separate PR.

cc @jmhbnz @ivanvc @ArkaSaha30 to take a second look.

ivanvc

This PR is looking good. I left two minor comments.

ivanvc · 2024-05-24T23:38:23Z

server/storage/mvcc/hash.go

@@ -111,7 +111,7 @@ type hashStorage struct {
 	lg     *zap.Logger
 }

-func newHashStorage(lg *zap.Logger, s *store) *hashStorage {
+func NewHashStorage(lg *zap.Logger, s *store) *hashStorage {


This (now) exported function returns an unexported struct. According to golangci-lint (and a linter rule we still need to enable), this is not a good practice. Refer to golang/lint#210.

However, HashStorage is already defined as the interface. So, addressing this may require renaming either the interface or the struct 🤔. This may open a discussion rather than an immediate action item.

Updated to return the interface.

ivanvc · 2024-05-24T23:43:16Z

etcdutl/etcdutl/hashkv_command.go

+	CompactRevision int64  `json:"compactRevision"`
+}
+
+func calculateHashKV(dbPath string, rev int64) (ds HashKV, err error) {


Nit: I don't think this function benefits from defining named return values. ds is not used, and err is defined in line 70.

Removed return var names.

Signed-off-by: Cenk Alti <cenkalti@gmail.com> Apply suggestions from code review Co-authored-by: Benjamin Wang <benjamin.wang@broadcom.com>

ahrtr

LGTM

Thanks. Please also add a changelog item for 3.6 in a separate PR.

ivanvc

LGTM. Thanks, @cenkalti.

cenkalti force-pushed the hashkv branch from 65b95c9 to 5334889 Compare May 26, 2023 19:55

cenkalti requested review from ptabor and ahrtr May 26, 2023 19:58

cenkalti added the area/etcdutl label May 26, 2023

cenkalti requested a review from chaochn47 May 26, 2023 19:59

cenkalti force-pushed the hashkv branch 2 times, most recently from 5eb6ff6 to 2b5fffb Compare May 26, 2023 20:06

jmhbnz reviewed May 26, 2023

View reviewed changes

cenkalti force-pushed the hashkv branch from 2b5fffb to 1873518 Compare May 26, 2023 21:07

jmhbnz approved these changes May 26, 2023

View reviewed changes

ahrtr added the type/feature label May 26, 2023

chaochn47 reviewed May 31, 2023

View reviewed changes

etcdutl/snapshot/v3_snapshot.go Outdated Show resolved Hide resolved

serathius approved these changes Jun 1, 2023

View reviewed changes

ahrtr reviewed Jun 2, 2023

View reviewed changes

etcdutl/etcdutl/printer.go Outdated Show resolved Hide resolved

etcdutl/etcdutl/snapshot_command.go Outdated Show resolved Hide resolved

etcdutl/etcdutl/snapshot_command.go Outdated Show resolved Hide resolved

etcdutl/etcdutl/snapshot_command.go Outdated Show resolved Hide resolved

ahrtr requested changes Jun 2, 2023

View reviewed changes

ahrtr reviewed Jun 2, 2023

View reviewed changes

stale bot added the stale label Sep 17, 2023

stale bot removed the stale label Jan 16, 2024

cenkalti force-pushed the hashkv branch from 1873518 to 8d8f546 Compare January 23, 2024 19:02

cenkalti force-pushed the hashkv branch 3 times, most recently from 2de11cb to 608d191 Compare March 30, 2024 14:32

jmhbnz requested a review from ahrtr March 30, 2024 19:59

ahrtr reviewed May 23, 2024

View reviewed changes

ivanvc reviewed May 24, 2024

View reviewed changes

cenkalti force-pushed the hashkv branch from 608d191 to 80594e2 Compare June 2, 2024 03:59

cenkalti requested review from ahrtr and ivanvc June 2, 2024 04:00

cli: Add etcdutl snapshot hashkv command

b107d24

Signed-off-by: Cenk Alti <cenkalti@gmail.com> Apply suggestions from code review Co-authored-by: Benjamin Wang <benjamin.wang@broadcom.com>

cenkalti force-pushed the hashkv branch from 80594e2 to b107d24 Compare June 2, 2024 04:07

ahrtr approved these changes Jun 3, 2024

View reviewed changes

ivanvc approved these changes Jun 3, 2024

View reviewed changes

ahrtr merged commit dc8510c into etcd-io:main Jun 3, 2024
46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: Add etcdutl snapshot hashkv command #15965

cli: Add etcdutl snapshot hashkv command #15965

cenkalti commented May 26, 2023 •

edited

Loading

jmhbnz left a comment

cenkalti commented May 26, 2023

jmhbnz left a comment

ahrtr left a comment

ahrtr Jun 2, 2023

stale bot commented Sep 17, 2023

cenkalti commented Jan 19, 2024

jmhbnz commented Mar 28, 2024

cenkalti commented Mar 28, 2024

cenkalti commented Mar 30, 2024

ahrtr May 23, 2024

cenkalti Jun 2, 2024

ahrtr commented May 23, 2024

ivanvc left a comment

ivanvc May 24, 2024

cenkalti Jun 2, 2024

ivanvc May 24, 2024

cenkalti Jun 2, 2024

ahrtr left a comment

ivanvc left a comment

cli: Add etcdutl snapshot hashkv command #15965

cli: Add etcdutl snapshot hashkv command #15965

Conversation

cenkalti commented May 26, 2023 • edited Loading

jmhbnz left a comment

Choose a reason for hiding this comment

cenkalti commented May 26, 2023

jmhbnz left a comment

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr Jun 2, 2023

Choose a reason for hiding this comment

stale bot commented Sep 17, 2023

cenkalti commented Jan 19, 2024

jmhbnz commented Mar 28, 2024

cenkalti commented Mar 28, 2024

cenkalti commented Mar 30, 2024

ahrtr May 23, 2024

Choose a reason for hiding this comment

cenkalti Jun 2, 2024

Choose a reason for hiding this comment

ahrtr commented May 23, 2024

ivanvc left a comment

Choose a reason for hiding this comment

ivanvc May 24, 2024

Choose a reason for hiding this comment

cenkalti Jun 2, 2024

Choose a reason for hiding this comment

ivanvc May 24, 2024

Choose a reason for hiding this comment

cenkalti Jun 2, 2024

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

ivanvc left a comment

Choose a reason for hiding this comment

cenkalti commented May 26, 2023 •

edited

Loading