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

Switch to explicit atomic_batch() database handle #1304

Merged
merged 3 commits into from Sep 18, 2018

Conversation

carver
Copy link
Contributor

@carver carver commented Sep 18, 2018

What was wrong?

The old approach of batching all changes made while leveldb was locked was flawed in a multi-threading context: it permits unintended writes to be batched with the ones you want.

How was it fixed?

Return an explicit handle when making an atomic batch, so that all the batched changes you want to make are explicitly part of that atomic batch. eg~

atomic_db = AtomicDB()
with atomic_db.atomic_batch() as db:
    # changes are not immediately saved to the db, inside this context
    db[key] = val

    # changes are still locally visible even though they are not yet committed to the db
    assert db[key] == val

    # changes using the original db happen instantly, and are visible in the batch db
    atomic_db[key2] = val2
    assert db[key2] == val2

Cute Animal Picture

put a cute animal picture link inside the parentheses

@carver
Copy link
Contributor Author

carver commented Sep 18, 2018

We should expect performance gains once we start using write batches, as well.

self._write_batch.delete(key)
del self._track_diff[key]

def shutdown(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename to decomission

@@ -37,6 +46,136 @@ def test_set_and_get(memory_db, level_db):
assert level_db.get(b'1') == memory_db.get(b'1')


def test_atomic_batch(level_db, atomic_db):
Copy link
Member

Choose a reason for hiding this comment

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

Rather than duplicating these here, do you think it makes sense to make this test suite generic or parametrized so that we can confirm feature parity between our in-memory databases and the leveldb ones?

eth/db/atomic.py Outdated

@classmethod
@contextmanager
def commit_unexceptional(cls, write_target_db):
Copy link
Member

Choose a reason for hiding this comment

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

This name is odd, but am I correct that it's an internal API. If so, then I don't really care that much other than maybe prefixing with an _ to make that clear.

@carver carver merged commit 683c53c into ethereum:master Sep 18, 2018
@carver carver deleted the leveldb-atomic branch September 18, 2018 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants