Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: add snapshot package #9118

Merged
merged 4 commits into from
Jan 23, 2018
Merged

*: add snapshot package #9118

merged 4 commits into from
Jan 23, 2018

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jan 8, 2018

Fix #8632.
Address #9109 with tests in snapshot package.

@gyuho gyuho requested a review from xiang90 January 8, 2018 18:27
@gyuho gyuho force-pushed the snapshot-code branch 2 times, most recently from 2b0e785 to fe02424 Compare January 8, 2018 19:12
@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #9118 into master will decrease coverage by 0.03%.
The diff coverage is 56.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9118      +/-   ##
==========================================
- Coverage   75.89%   75.86%   -0.04%     
==========================================
  Files         359      361       +2     
  Lines       30054    30153      +99     
==========================================
+ Hits        22810    22876      +66     
- Misses       5650     5678      +28     
- Partials     1594     1599       +5
Impacted Files Coverage Δ
etcdctl/ctlv3/command/printer_table.go 0% <0%> (ø) ⬆️
etcdctl/ctlv3/command/printer_simple.go 70.62% <0%> (ø) ⬆️
etcdctl/ctlv3/command/printer.go 43.26% <0%> (-0.55%) ⬇️
etcdctl/ctlv3/command/printer_fields.go 0% <0%> (ø) ⬆️
snapshot/logger.go 21.21% <21.21%> (ø)
etcdctl/ctlv3/command/printer_json.go 58.33% <33.33%> (ø) ⬆️
etcdctl/ctlv3/command/snapshot_command.go 67.32% <54.76%> (+1.98%) ⬆️
snapshot/v3_snapshot.go 64.7% <64.7%> (ø)
etcdctl/ctlv2/command/watch_command.go 57.5% <0%> (-22.5%) ⬇️
pkg/adt/interval_tree.go 88.28% <0%> (-4.21%) ⬇️
... and 24 more

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 202cc9a...6fcee6b. Read the comment docs.

@xiang90
Copy link
Contributor

xiang90 commented Jan 10, 2018

it seems that snapshot should be a sub pkg under etcdctl?

@xiang90
Copy link
Contributor

xiang90 commented Jan 10, 2018

/cc @Quentin-M can you give it a look? is this what you want?

@gyuho gyuho added the WIP label Jan 10, 2018
@Quentin-M
Copy link
Contributor

That's awesome! Thank you so much!

logger grpclog.LoggerV2
}

func (s *v3Snapshotter) Save(ctx context.Context, cli *clientv3.Client, dbPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

the client should be part of the config i feel, not per save.

@gyuho gyuho added the WIP label Jan 19, 2018
@gyuho gyuho removed the WIP label Jan 22, 2018
@gyuho gyuho changed the title *: add snapshot package with restore tests *: add snapshot package Jan 22, 2018
)

// DefaultLogger defines default snapshot logger based on "capnslog".
var DefaultLogger grpclog.LoggerV2
Copy link
Contributor

Choose a reason for hiding this comment

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

this probably should not belong to snapshot pkg. it is not snapshot related.

)

// Snapshotter defines snapshot methods.
type Snapshotter interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to Manager? so it looks like snapshot.Manager.Save/Status/Restore

}

// DBStatus is the snapshot file status.
type DBStatus struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just Status? Hash/Revision is not db (boltdb in our case) specific, but snapshot related.

}

// Config defines snapshot config.
type Config struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably should kill this config struct. this is not snapshot related configuration.

return ds, nil
}

type revision struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this to util.go?


// initIndex implements ConsistentIndexGetter so the snapshot won't block
// the new raft instance by waiting for a future raft index.
type initIndex int
Copy link
Contributor

Choose a reason for hiding this comment

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

move to util.go?

@xiang90
Copy link
Contributor

xiang90 commented Jan 22, 2018

a few nits. look good to me in general.

@gyuho gyuho added the WIP label Jan 22, 2018
@gyuho
Copy link
Contributor Author

gyuho commented Jan 22, 2018

@xiang90 PTAL. All fixed.

@xiang90
Copy link
Contributor

xiang90 commented Jan 23, 2018

lgtm once CI gets passed.

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants