Base implementation of RocksDB support by pipermerriam · Pull Request #1416 · ethereum/py-evm

pipermerriam · 2018-10-19T19:39:35Z

What was wrong?

LevelDB doesn't support having multiple processes reading from the same database. We worked around this by having a dedicated database process which has a direct handle on the database and all other processes interact with it over multiprocessing pipes.

This is not ideal. It incurs extra overhead both with respect to performance and code complexity.

How was it fixed?

Implemented a RocksDB backend which uses an alternate underlying database called rocksdb -> https://rocksdb.org/

This PR establishes the database backend class and adds a CLI argument to allow opt-in use of the new database backend. A subsequent PR will tackle removal of the database process entirely.

Cute Animal Picture

pipermerriam · 2018-10-19T19:40:24Z

    def __delitem__(self, key: bytes) -> None:
+        v = self.db.get(key)
+        if v is None:
+            raise KeyError(key)


Without this the leveldb backend isn't fully compliant with the BaseDB api.

pipermerriam · 2018-10-19T19:42:39Z

-def test_set_and_get(memory_db, level_db):
-    level_db.set(b'1', b'1')
-    memory_db.set(b'1', b'1')
-    assert level_db.get(b'1') == memory_db.get(b'1')


These were removed as it appeared all of these tests are covered by test_base_db_api.py

carver · 2018-10-19T19:59:41Z

+def run_database_process(trinity_config: TrinityConfig) -> None:
    with trinity_config.process_id_file('database'):
-        base_db = db_class(db_path=trinity_config.database_dir)
+        base_db = trinity_config.db_class(db_path=trinity_config.database_dir)


I think rocks and level use names that prevent filepath collisions, but worth double checking.

I hadn't gotten this far but yes, we should be very sure that we cannot re-use a database directory that was previously used with another backend.

I thought about fixing this by namespacing the directory based on the backend but... then we could get into a situation where the user switches the backend midstream and effectively has two versions of the same chain.

So maybe just a small file we place somewhere to be able to check what database is being used.

TODO: make sure leveldb and rocksdb databases cannot use the same root directory

carver · 2018-10-19T22:03:04Z

          sudo apt-get install -y liblz4-dev libsnappy-dev libgflags-dev zlib1g-dev libbz2-dev libzstd-dev
          if [ ! -f "/root/project/rocksdb/librocksdb.a" ]; then
            git clone https://github.com/facebook/rocksdb
-            cd rocksdb/ && git checkout v5.8.8 && make install-shared INSTALL_PATH=/usr


Haha, where are those bash aliases when you need them?

Sign in to view



 def test_database_api_missing_key_for_deletion(db):
-    db.delete(b'does-not-exist')


carver · 2018-10-22T18:20:45Z

(After CircleCI kicks off and goes green, of course, plus some commit squashing)

pipermerriam

still need to ensure we don't have rocksdb based databases and leveldb based databases in the same path.

pipermerriam · 2018-10-23T01:18:25Z

+def run_database_process(trinity_config: TrinityConfig) -> None:
    with trinity_config.process_id_file('database'):
-        base_db = db_class(db_path=trinity_config.database_dir)
+        base_db = trinity_config.db_class(db_path=trinity_config.database_dir)


TODO: make sure leveldb and rocksdb databases cannot use the same root directory

pipermerriam · 2018-10-23T16:52:36Z

Alright, last thing is to ensure that the rocksdb build is properly cached (since it takes about 8-9 minutes to build and we don't want to do that in every ci run).

pipermerriam · 2018-10-23T21:41:21Z

I CURRENTLY HATE EVERYTHING. The lightchain_integration tests are failing in ways that they shouldn't..... grumble.

carver

Mostly just nitpicks/questions about naming, yagni, docs. GTG

The tox -r change is the one I'd have to think about more if you want to keep as is in the PR.

carver · 2018-10-23T22:48:49Z

@@ -1,4 +1,4 @@
-version: 2.0
+version: 2.1


Just curious, what required this upgrade?

It's superfluous. I should remove it.

carver · 2018-10-23T22:51:04Z

          geth version
    - run:
        name: run tox
-        command: ~/.local/bin/tox -r


I think this was helping us quickly catch when a live change in a dependency was causing problems with py-evm. Was the change for performance? How much shorter was the test?

Another thing that didn't get reverted. will remove.

carver · 2018-10-23T22:53:31Z

+#!/usr/bin/env bash
+
+set -e
+set -u


I find the long-form nicer for readability, like:

set -o errexit set -o nounset set -o pipefail

TIL there is a longform.

carver · 2018-10-23T22:55:38Z

+
+.. code:: sh
+
+  apt-get install liblz4-dev lib-rocksdb5.8


Did this doc get stale? I suppose there's a reason you had to switch circle to use:

sudo apt-get install -y liblz4-dev libsnappy-dev libgflags-dev zlib1g-dev libbz2-dev libzstd-dev

Not stale. Requirements for rocksdb installation, will double check but I believe these are directly from their readme.

On a related note, I think our installation story is about to get ungood and we should look into debian packages and/or brew. We're gonna have to do it eventually...

carver · 2018-10-23T22:56:44Z

+        self.db.delete(key)
+
+    @contextmanager
+    def atomic_batch(self) -> Generator['RocksDBWriteBatch', None, None]:


Iterator['RocksDBWriteBatch']?

likely copy/paste from LevelDB. Will change.

carver · 2018-10-23T23:02:26Z

+
+    @property
+    def is_db_backend_rocksdb(self) -> bool:
+        return self.db_backend == DB_ROCKS


I want to say YAGNI about these is_db_b... properties, and go back to just running these lines in the if test internally. Do we lose much by dropping the methods?

I wrote these because I don't want to have to import the DB_ROCKS and DB_LEVEL constants in the places that these checks happen. I ended up doing the same with is_full_sync and is_fast_sync. Unless you feel strongly I'd like to leave them.

carver · 2018-10-23T23:10:46Z


+    # Verify that the database engine that trinity is configured to use matches
+    # the existing on-disk engine.
+    if trinity_config.db_backend != trinity_config.on_disk_database_engine:


Since both db_backend and database_engine are storing those constants in {'rocks', 'level'}, can we unify the names a bit? I think I like engine better, because backend implies to me that I'm going to get a LevelDB or LevelDB() back instead of 'level'.

So maybe config.db_engine and config.on_disk_db_engine?

carver · 2018-10-23T23:15:21Z

+        )
+
+
+DATABASE_ENGINE_LOCK_NAME = '.trinity.db_engine.lock'


I see what you're saying with lock, but also I'm used to lock files being things that should go away after shutdown.

Maybe?

.trinity.db_engine.freeze

.trinity.db_engine.frozen

.trinity.db_engine.initialized

.trinity.db_engine.selected

(and all the other places that use lock for this choice)

went with marker

This reverts commit 61a9a37.

pipermerriam added the PR state: WIP label Oct 19, 2018

pipermerriam commented Oct 19, 2018

View reviewed changes

carver reviewed Oct 19, 2018

View reviewed changes

Comment thread tests/database/test_base_db_api.py

def test_database_api_missing_key_for_deletion(db):

db.delete(b'does-not-exist')

This comment was marked as resolved.

Sign in to view

carver approved these changes Oct 22, 2018

View reviewed changes

pipermerriam force-pushed the piper/rocksdb branch 2 times, most recently from cc8a176 to 0b61dd2 Compare October 22, 2018 21:41

pipermerriam commented Oct 23, 2018

View reviewed changes

pipermerriam added 2 commits October 23, 2018 15:17

Add RocksDB backend

3c114a4

Ensure we dont reuse database dir for different database engines

f66ba87

pipermerriam force-pushed the piper/rocksdb branch from 767c1a9 to 57d2164 Compare October 23, 2018 21:27

Cache rocksdb installation

0b1a67b

pipermerriam force-pushed the piper/rocksdb branch from 1c18f54 to 0b1a67b Compare October 23, 2018 22:01

pipermerriam added PR state: needs review type: refactor effort: E2 days and removed PR state: WIP labels Oct 23, 2018

carver approved these changes Oct 23, 2018

View reviewed changes

pipermerriam added 2 commits October 24, 2018 09:46

PR feedback

03bfb48

fix bash scripts

3279e60

pipermerriam mentioned this pull request Oct 24, 2018

Complete Type Hinting for eth.tools #1420

Merged

fix types

6f5b702

pipermerriam merged commit 61a9a37 into ethereum:master Oct 24, 2018

pipermerriam deleted the piper/rocksdb branch October 24, 2018 19:22

pipermerriam mentioned this pull request Oct 25, 2018

Christoph/review/1420 #1430

Closed

cburgdorf added a commit to cburgdorf/py-evm that referenced this pull request Oct 25, 2018

Revert "Base implementation of RocksDB support (ethereum#1416)"

24b48d8

This reverts commit 61a9a37.

cburgdorf added a commit that referenced this pull request Oct 25, 2018

Revert "Base implementation of RocksDB support (#1416)"

88bbed3

This reverts commit 61a9a37.



		def test_database_api_missing_key_for_deletion(db):
		db.delete(b'does-not-exist')

		)


		DATABASE_ENGINE_LOCK_NAME = '.trinity.db_engine.lock'

Conversation

pipermerriam commented Oct 19, 2018

What was wrong?

How was it fixed?

Cute Animal Picture

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

carver commented Oct 22, 2018

Uh oh!

pipermerriam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipermerriam commented Oct 23, 2018

Uh oh!

pipermerriam commented Oct 23, 2018

Uh oh!

carver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pipermerriam left a comment •

edited

Loading