Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

add support to rocksdb #151

Merged
merged 4 commits into from
Jan 9, 2018
Merged

Conversation

xingfang0408
Copy link

What current issue(s) does this address?, or what feature is it adding?
add rocksdb support in neo/Implementations/Blockchains/RocksDB/.

How did you solve this problem?
add a new rocksdb directory and replaced existing API calls to leveldb with rocksdb in the new directory.
add prompt_rocksdb.py under root to invoke rocksdb APIs.

How did you make sure your solution works?
Executed existing testcases under leveldb/test, tests passed, no regression.

Did you add any tests?
Migrated existing testcases under LevelDB/test to RocksDB/test, tests passed.

Are there any special changes in the code that we should be aware of?
N/A. User need to take separate steps to install rocksdb python package.

@localhuman
Copy link
Collaborator

Initial read, this is great! Could you close this PR and make PR on development branch?

Thanks!

@xingfang0408 xingfang0408 changed the base branch from master to development January 4, 2018 18:37
@metachris
Copy link
Contributor

metachris commented Jan 4, 2018

That's quite a pull request, I'm sure that took a lot of effort! Thanks for contributing.

A few remarks and questions:

  • Please replace all print statements with logzero.info or logzero.debug (no print statements are allowed outside of prompt)

  • There are some minor issues with the codestyle, which you can see when you run pycodestyle neo.

  • Do those tests cover all the test cases that run with the leveldb also with rocksdb? I guess it would be great to run the tests as thoroughly as possible with rocksdb just as with leveldb. cc/ @localhuman

  • Why do you copy prompt.py to prompt_rocksdb.py? are there any changes besides the used db? i would try to add pluggable DB support to the one prompt.py we already have, else we need to maintain two files for all changes to prompt. perhaps a simple cli argument could be used to set which db to use.

    For switching db implementation in prompt.py, you could for now just add a --rocks-db argument here https://github.com/CityOfZion/neo-python/blob/master/prompt.py#L842, and a few lines below check if this is provided and then setup rocksdb instead of leveldb here https://github.com/CityOfZion/neo-python/blob/master/prompt.py#L866

@metachris
Copy link
Contributor

Could you please target the branch feature-rocksdb with this pull request? Using this branch as a step before development will make it easier to merge this PR, testing and putting on the finishing touches, before merging into development.

In my opinion, as soon as pycodestyle neo tests runs without errors, and the print statements are replaced by logzero.* calls, it can be merged into the feature branch. Then in a next step we target the tests, prompt.py and ways of switching between the db implementations.

<3

@xingfang0408 xingfang0408 changed the base branch from development to feature-rocksdb January 6, 2018 01:28
@localhuman
Copy link
Collaborator

Looks good! One last thing, can you run pycodestyle on prompt.py? Then the tests will pass :)

@xingfang0408
Copy link
Author

pycodestyle prompt.py or prompt_rocksdb.py didn't error out.

@metachris
Copy link
Contributor

Right, the tests fail because the rocksdb package is not present:

ERROR: Implementations.Blockchains.RocksDB.test.test_initial_db (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: Implementations.Blockchains.RocksDB.test.test_initial_db
Traceback (most recent call last):
File "/opt/python/3.5.4/lib/python3.5/unittest/loader.py", line 428, in _find_test_path
    module = self._get_module_from_name(name)
File "/opt/python/3.5.4/lib/python3.5/unittest/loader.py", line 369, in _get_module_from_name
    __import__(name)
File "/home/travis/build/CityOfZion/neo-python/neo/Implementations/Blockchains/RocksDB/test/test_initial_db.py", line 2, in <module>
    from neo.Implementations.Blockchains.RocksDB.RocksDBBlockchain import RocksDBBlockchain
File "/home/travis/build/CityOfZion/neo-python/neo/Implementations/Blockchains/RocksDB/RocksDBBlockchain.py", line 3, in <module>
    import rocksdb
ImportError: No module named 'rocksdb'

I think this state is fine to merge it in the feature-rocksdb branch. Then we can move on to more testing and merging the prompt.py files.

👍

@metachris
Copy link
Contributor

@xingfang0408 which rocksdb Python package are you using? This: https://github.com/stephan-hof/pyrocksdb?

@xingfang0408
Copy link
Author

@metachris This is the rocksdb python package I use https://github.com/twmht/python-rocksdb, it looks like the other one hasn't been updated for a long time.

@localhuman
Copy link
Collaborator

Looks great! We'll merge it in and start work on integrating it to the project!

@metachris metachris merged commit a11cb9b into CityOfZion:feature-rocksdb Jan 9, 2018
@metachris metachris mentioned this pull request Jan 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants