Skip to content

RocksDB#1063

Closed
shargon wants to merge 53 commits intoneo-project:masterfrom
shargon:rocks-db
Closed

RocksDB#1063
shargon wants to merge 53 commits intoneo-project:masterfrom
shargon:rocks-db

Conversation

@shargon
Copy link
Copy Markdown
Member

@shargon shargon commented Aug 27, 2019 •
edited
Loading

@shargon shargon requested a review from erikzhang August 27, 2019 10:14
@shargon
Copy link
Copy Markdown
Member Author

shargon commented Aug 27, 2019 •
edited
Loading

@eryeer Could you move your LevelDB unit tests to this PR? (modified for RocksDB)

@shargon shargon added the Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. label Aug 27, 2019
shargon
shargon commented Aug 27, 2019
View reviewed changes
@vncoelho
Copy link
Copy Markdown
Member

vncoelho commented Aug 27, 2019 •
edited
Loading

@shargon, I think that the discussion #966 was not yet decided, please check my comments there.

In the last comment of @erikzhang he mentioned that it was not yet decided when you asked.
I also had a conversation with @jsolman and he also did not say that RockDB would be better.

In this sense, I think that this PR could wait a little more.
It will require a considerable effort. This is not a priority for NEO 3 I think.

The DB could be even more flexible, this would really be a good change.
By having the DB more flexible it could even by defined by choice of the node runner.

I think that a change like this would be more plausible with a dedicated benchmark when we have defined a strategy for testing the Blockchain and throughput.

I am not in favor of this change right now.

However, if you really think it is better in terms of performance and the needs, I think that you should push this forward, because you are more expert in this topic than me.
On the other hand, I think that we currently are not really able to test all cases statistically. But maybe it is clear to you that this is better.

@vncoelho vncoelho mentioned this pull request Aug 27, 2019
Closed
@shargon shargon mentioned this pull request Aug 27, 2019
Closed
vncoelho
vncoelho reviewed Aug 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@shargon, with the possibility of switching between them (and perhaps, for now, leaving LevelDB as default) I am 100% in favor of this.

In addition, it is was surely not an easy task to you and since you already dedicated time to learn and push this, I believe that it should be merged.

vncoelho
vncoelho reviewed Aug 27, 2019
View reviewed changes
vncoelho
vncoelho reviewed Aug 27, 2019
View reviewed changes
vncoelho
vncoelho reviewed Aug 27, 2019
View reviewed changes
vncoelho
vncoelho reviewed Aug 27, 2019
View reviewed changes
@vncoelho vncoelho added the Feature Type: Large changes or new features label Aug 27, 2019
@shargon
Copy link
Copy Markdown
Member Author

shargon commented Aug 29, 2019 •
edited
Loading

@eryeer wait until merge this then, we can do the test for the three storages, memory #1064, RocksDB and LevelDB together

@eryeer
Copy link
Copy Markdown
Contributor

eryeer commented Aug 30, 2019

@shargon Ok, after it merged, let's test them together.

erikzhang
erikzhang reviewed Aug 31, 2019
View reviewed changes
vncoelho
vncoelho approved these changes Sep 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Beautiful job @shargon!
Congratulations. Flexible and powerful.

This surely opens a wide path of possibilities.

@erikzhang
Copy link
Copy Markdown
Member

erikzhang commented Sep 6, 2019

Maybe we can move this to a plugin. Because I anticipate that we may have many different storages in the future, such as LevelDb, RocksDb, FASTER, and memory. If each one is integrated in the core, it is not necessary.

@vncoelho
Copy link
Copy Markdown
Member

vncoelho commented Sep 6, 2019

I agree, Erik.

We could merge @shargon refactor at least, yes?

vncoelho
vncoelho previously requested changes Sep 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vncoelho vncoelho left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The plugin sounds a good idea that @erikzhang suggested and even more flexible, promoting a compact code.

Maybe we can keep the refactoring of level leveldb.

@shargon, it is a great job and this was the most import step. Congratulations once more.
Thinking on Rocks, Level, Faster and Memory makes us more prone to consider as a plugin.

@vncoelho vncoelho dismissed their stale review September 6, 2019 16:22

dismissing because of possibility for changing to plugins. However, PR had my previous approval.

@shargon
Copy link
Copy Markdown
Member Author

shargon commented Sep 7, 2019

After #1087 i will reduce this PR for preserve only the refactored part, and move th rest to neo-plugins

@shargon shargon mentioned this pull request Nov 17, 2019
Merged
@shargon
Copy link
Copy Markdown
Member Author

shargon commented Nov 17, 2019

I will close this when IStorage is merged, in order to remember the improves. In favor of neo-project/neo-modules#144

@erikzhang
Copy link
Copy Markdown
Member

erikzhang commented Nov 18, 2019

Please review #1249 first.

@shargon shargon mentioned this pull request Nov 30, 2019
Closed
@shargon
Copy link
Copy Markdown
Member Author

shargon commented Nov 30, 2019

LevelDb changes moved to #1308
RocksDb changes moved to neo-project/neo-modules#144

@shargon shargon closed this Nov 30, 2019
@shargon shargon deleted the rocks-db branch November 30, 2019 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erikzhang erikzhang erikzhang approved these changes

@vncoelho vncoelho vncoelho left review comments

Assignees

@shargon shargon

Labels

Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. Feature Type: Large changes or new features Ledger Module - The ledger is our 'database', this is used to tag changes about how we store information

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use RocksDB?

6 participants

@shargon @vncoelho @eryeer @erikzhang @codecov-io @lock9