Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

add decode and load meta command#80

Closed
3pointer wants to merge 2 commits intopingcap:masterfrom
3pointer:debug_backupmeta
Closed

add decode and load meta command#80
3pointer wants to merge 2 commits intopingcap:masterfrom
3pointer:debug_backupmeta

Conversation

@3pointer
Copy link
Collaborator

@3pointer 3pointer commented Dec 4, 2019

No description provided.

@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #80 into master will decrease coverage by 1.03%.
The diff coverage is 14.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
- Coverage   66.75%   65.71%   -1.04%     
==========================================
  Files          22       22              
  Lines        2644     2701      +57     
==========================================
+ Hits         1765     1775      +10     
- Misses        642      687      +45     
- Partials      237      239       +2
Impacted Files Coverage Δ
pkg/utils/schema.go 55.88% <ø> (ø) ⬆️
cmd/meta.go 12.85% <14.03%> (+0.8%) ⬆️
pkg/restore/import.go 76.55% <0%> (+1.37%) ⬆️

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 e9bae2c...a88fa6f. Read the comment docs.

@kennytm kennytm requested a review from 5kbpers December 4, 2019 08:19
@kennytm
Copy link
Collaborator

kennytm commented Dec 4, 2019

cc @5kbpers. This PR conflicts with #78 which drops the entire meta subcommand. Please advise.

@5kbpers
Copy link
Contributor

5kbpers commented Dec 4, 2019

What do you think the better cmd name? I prefer validate or check since we have a package in src/pkg which has the same name with meta and it does not have any relationship with this command.
Or I can change the command added in #78 to meta validate.
cc @kennytm @overvenus

@kennytm
Copy link
Collaborator

kennytm commented Dec 4, 2019

Or maybe just store the backupmeta as JSON instead of protobuf so we don't need this command 😝

@5kbpers
Copy link
Contributor

5kbpers commented Dec 5, 2019

Or maybe just store the backupmeta as JSON instead of protobuf so we don't need this command 😝

Good idea! cc @3pointer @overvenus

@overvenus
Copy link
Member

Let's keep protobuf format for production, JSON number makes me uncomfortable. :-/

I think this command is good enough for debug/hotfix purposes.

return nil
},
}
meta.AddCommand(decodeBackupMetaCmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you test it in the integration tests? It can improve code coverage.

return nil
},
}
meta.AddCommand(loadBackupMetaCmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you test it in the integration tests? It can improve code coverage.

Co-Authored-By: Neil Shen <overvenus@gmail.com>
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some tests? Rest LGTM


loadBackupMetaCmd := &cobra.Command{
Use: "encode",
Short: "load backupmeta json file to backupmeta",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Short: "load backupmeta json file to backupmeta",
Short: "encode backupmeta json file to backupmeta",

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants