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

RocksDB #1063

Closed
wants to merge 53 commits into from
Closed

RocksDB #1063

wants to merge 53 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Aug 27, 2019

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

shargon commented Aug 27, 2019

@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
@vncoelho
Copy link
Member

vncoelho commented Aug 27, 2019

@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.

Copy link
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.

neo/IO/Data/RocksDB/DB.cs Outdated Show resolved Hide resolved
neo/IO/Data/RocksDB/DB.cs Outdated Show resolved Hide resolved
neo/IO/Data/RocksDB/Options.cs Show resolved Hide resolved
neo/IO/Data/Slice.cs Outdated Show resolved Hide resolved
@vncoelho vncoelho added the Feature Type: Large changes or new features label Aug 27, 2019
@shargon
Copy link
Member Author

shargon commented Aug 29, 2019

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

@eryeer
Copy link
Contributor

eryeer commented Aug 30, 2019

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

neo/neo.csproj Outdated Show resolved Hide resolved
Copy link
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
Member

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
Member

vncoelho commented Sep 6, 2019

I agree, Erik.

We could merge @shargon refactor at least, yes?

Copy link
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.

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
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
Copy link
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
Member

Please review #1249 first.

@shargon shargon mentioned this pull request Nov 30, 2019
@shargon
Copy link
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
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
Development

Successfully merging this pull request may close these issues.

Use RocksDB?
6 participants