Skip to content

Add snapshot and checkpoint #216

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

Closed
wants to merge 10 commits into from
Closed

Add snapshot and checkpoint #216

wants to merge 10 commits into from

Conversation

jchiu0
Copy link
Contributor

@jchiu0 jchiu0 commented Sep 15, 2016

Checkpoint is new: it is not found in RocksDB C API or gorocksdb.


This change is Reviewable

cDb *C.rdb_t
}

// Release removes the snapshot from the database's list of snapshots.

Choose a reason for hiding this comment

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

comment on exported method Checkpoint.Destroy should be of the form "Destroy ..."

@coveralls
Copy link

Coverage Status

Coverage remained the same at 54.483% when pulling 4512caf on feature/rdb into 0ae39a7 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 54.483% when pulling 4512caf on feature/rdb into 0ae39a7 on master.

func (s *Store) Close() {
s.db.Close()
}
func (s *Store) Close() { s.db.Close() }

Choose a reason for hiding this comment

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

exported method Store.Close should have comment or be unexported

func (s *Store) Close() {
s.db.Close()
}
func (s *Store) Close() { s.db.Close() }

Choose a reason for hiding this comment

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

exported method Store.Close should have comment or be unexported

func (s *Store) Close() {
s.db.Close()
}
func (s *Store) Close() { s.db.Close() }

Choose a reason for hiding this comment

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

exported method Store.Close should have comment or be unexported


// Memtable returns the memtable size.

Choose a reason for hiding this comment

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

comment on exported method Store.MemtableSize should be of the form "MemtableSize ..."

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 54.555% when pulling 1d11db0 on feature/rdb into 10ccb1b on master.

@jchiu0
Copy link
Contributor Author

jchiu0 commented Sep 19, 2016

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.


rdb/checkpoint.go, line 18 at r1 (raw file):

Previously, houndci-bot (Hound) wrote…

comment on exported method Checkpoint.Destroy should be of the form "Destroy ..."

Done.

store/store.go, line 114 at r3 (raw file):

Previously, houndci-bot (Hound) wrote…

exported method Store.Close should have comment or be unexported

Done.

store/store.go, line 112 at r5 (raw file):

Previously, houndci-bot (Hound) wrote…

comment on exported method Store.MemtableSize should be of the form "MemtableSize ..."

Done.

Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 54.555% when pulling 1d11db0 on feature/rdb into 10ccb1b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 54.555% when pulling 1d11db0 on feature/rdb into 10ccb1b on master.

@@ -34,3 +34,7 @@ func (opts *ReadOptions) Destroy() {
func (opts *ReadOptions) SetFillCache(value bool) {
C.rdb_readoptions_set_fill_cache(opts.c, boolToChar(value))
}

func (opts *ReadOptions) SetSnapshot(snapshot *Snapshot) {

Choose a reason for hiding this comment

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

exported method ReadOptions.SetSnapshot should have comment or be unexported

@@ -34,3 +34,7 @@ func (opts *ReadOptions) Destroy() {
func (opts *ReadOptions) SetFillCache(value bool) {
C.rdb_readoptions_set_fill_cache(opts.c, boolToChar(value))
}

func (opts *ReadOptions) SetSnapshot(snapshot *Snapshot) {

Choose a reason for hiding this comment

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

exported method ReadOptions.SetSnapshot should have comment or be unexported

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 54.532% when pulling 9b11d8e on feature/rdb into 10ccb1b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 54.532% when pulling f8cd2d1 on feature/rdb into 10ccb1b on master.

@jchiu0
Copy link
Contributor Author

jchiu0 commented Sep 19, 2016

Updated the API. Added some tests to illustrate how to use snapshots and checkpoints. Please see store_test.go.


Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 54.595% when pulling 611b36c on feature/rdb into 10ccb1b on master.

@manishrjain
Copy link
Contributor

:lgtm: Haven't looked too deep, hope there are no memory leaks.


Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 2 files at r3, 1 of 2 files at r5, 2 of 4 files at r6, 1 of 1 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


rdb/snapshot.go, line 15 at r2 (raw file):

// NewNativeSnapshot creates a Snapshot object.
func NewNativeSnapshot(c *C.rdb_snapshot_t, cDb *C.rdb_t) *Snapshot {

Does Snapshot have any APIs?


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 54.595% when pulling 2b23935 on feature/rdb into 10ccb1b on master.

@jchiu0
Copy link
Contributor Author

jchiu0 commented Sep 19, 2016

All the memory that is allocated is wrapped by Go objects, e.g., Snapshot, Checkpoint. The user is responsible for calling Close/Destroy/Release methods of these Go objects unless we want to use runtime.SetFinalizer. Right now, we don't use runtime.SetFinalizer for such wrapped C objects and I think we should be consistent about it.

Thanks for the review.


Review status: 7 of 8 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


rdb/snapshot.go, line 15 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Does Snapshot have any APIs?

The snapshot API is very small: https://github.com/facebook/rocksdb/blob/522de4f59e6314698286cf29d8a325a284d81778/include/rocksdb/snapshot.h

The checkpoint API is also very small: https://github.com/facebook/rocksdb/blob/56887f6cb854fde8e16c1ca65533716976c2e2f3/utilities/checkpoint/checkpoint.cc

I think I have exposed all the major functions. store_test.go also illustrates that we have all we need to save and restore the database using either snapshots or checkpoints.


Comments from Reviewable

@jchiu0 jchiu0 closed this Sep 19, 2016
@jchiu0 jchiu0 deleted the feature/rdb branch September 19, 2016 13:10
arijitAD pushed a commit that referenced this pull request Oct 15, 2020
…ssage encoding/decoding (#216)

* update SCALE to encode uint64/int64 as LE, also add encoding of common.Hash ([32]byte) type
* improve p2p message encoding/decoding using updated SCALE
* add getExistingStream function which checks if an existing stream is open to a peer to prevent opening another one
* add ConnManager which monitors changes to connections (currently only logs opening/closing of polkadot streams)
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