Skip to content

Key-value DB using gob for encoding/decoding#88

Open
vqhuy wants to merge 1 commit into
masterfrom
kvdb
Open

Key-value DB using gob for encoding/decoding#88
vqhuy wants to merge 1 commit into
masterfrom
kvdb

Conversation

@vqhuy
Copy link
Copy Markdown
Member

@vqhuy vqhuy commented Sep 21, 2016

I have made an improvement in commit 84fc324bc27fee50c0619a9538c0d91421b804d1. This implementation's performance is better than #37 now.

Update (description below)

This pull uses encoding/gob to encode/decode the merkletree structs to/from a buffer, and it implements the db backend hook for the merkletree package.

The package merkletree is now exporting some functions for encoding/decoding the PAD, STR, Merkletree and Policies from a buffer (any types that implement the io.Writer and io.Reader interface). This way, I think we can hide the detail implementations of the encoding/decoding from the developers.

Move to the hook implementation, there are two cases when we want to use a persistent storage:

  • Store the current directory (pad).
  • Store the old STRs.

This hook exports APIs for store/load the PAD, STR to/from the db.

@vqhuy vqhuy added this to the 0.1.0 milestone Sep 21, 2016
@vqhuy vqhuy self-assigned this Sep 21, 2016
@vqhuy vqhuy force-pushed the kvdb branch 2 times, most recently from 84fc324 to 0203249 Compare September 22, 2016 10:32
@vqhuy vqhuy mentioned this pull request Sep 22, 2016
Copy link
Copy Markdown
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Looking good. I added a few questions. Hope to do a more detailed review before our hangouts session later.

Comment thread storage/kv/merkletreekv/storage.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, this is a old commit that has been deleted.

Comment thread merkletree/node.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess all this values need to public to correctly encode now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, since we didn't export the structs so I think it still ensures our preservation, i.e., there's no way to change or read the node's fields.
Btw, the reason is (mostly) because I didn't want to implement the GobEncode/GobDecode interface manually.

@vqhuy vqhuy force-pushed the kvdb branch 2 times, most recently from ecf430d to 28a53ac Compare September 22, 2016 12:12
Copy link
Copy Markdown
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Some suggestions/questions.

Comment thread merkletree/policy.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the comment not relevant anymore?

Comment thread merkletree/merkletree.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nitpicking: We would need this naming to be consistent. I liked the identifier (we already have so many different "keys" ;-) Here, we still stick with identifier: https://github.com/coniks-sys/coniks-go/pull/88/files#diff-7e689735c327b0bd737cd6e956e20b4aR4
Anyways, choose what you prefer. I just think it should be consistent.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, it must be my mistake. I have no intention to change this! Thanks!

Comment thread storage/kv/merkletreekv/strkv.go Outdated
Copy link
Copy Markdown
Member

@liamsi liamsi Sep 22, 2016

Choose a reason for hiding this comment

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

This could be unexported or not? If this should stay exported it would need a more speaking name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you have any suggestion for this? :D

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it is unexported the name is OK.

Comment thread storage/kv/merkletreekv/strkv.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This and LoadSTR currently aren't used. I guess this will change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They would be used to load the old STRs which were deleted from the pad.snapshot.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool, thanks. Can we have a test for that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup!

@liamsi
Copy link
Copy Markdown
Member

liamsi commented Sep 22, 2016

From my point of view this PR looks very good (just a few questions and nitpicks).

@vqhuy
Copy link
Copy Markdown
Member Author

vqhuy commented Sep 22, 2016

Thanks!
Update: I made some changes in e8e9169c14efeb33810e1053b8f92d9a160d691e according to @liamsi's comments.

Copy link
Copy Markdown
Member

@masomel masomel left a comment

Choose a reason for hiding this comment

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

The encoding, decoding and reconstruction functions aren't part of the core data structure, they are only relevant to the DB, and they are ultimately only used in EncodePAD, which is really only used by StorePAD in padkv.go. As such, all gob encoding and decoding functions related to the DB should be part of the storage/kv/merkletreekv package, not the core merkletree package.

I agree that the encoding format is an implementation detail we should hide from the developer, but I don't see the reason why these functions need to be part of the core data structure if they are used only for a separate feature (the DB) of coniks-go.

liamsi
liamsi previously approved these changes Oct 16, 2016
@vqhuy
Copy link
Copy Markdown
Member Author

vqhuy commented Dec 21, 2016

PTAL. I need to use this in extensions implementation ;)

@vqhuy vqhuy force-pushed the kvdb branch 3 times, most recently from 74739cb to 91590e5 Compare December 21, 2016 14:40
- Export fields of nodes struct for gob encoding

- Add gob encoding/decoding for PAD, STR, MerkleTree, Policies (use internally)
@vqhuy vqhuy added this to the 0.3.0 milestone Apr 12, 2017
@vqhuy vqhuy removed this from the 0.2.0 milestone Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants