Blockchain: refactor: Add abstract DB operation, let DB manage the implementation of this OP + make DB handle Cache #927

jochem-brouwer · 2020-10-28T18:54:59Z

Blockchains index.ts used to do a lot of DB-like operations, like calculating the keys and updating the cache. This is now different; a databaseOperation factory is added, which describes these operations. Also, all DB operations are moved into a DB folder. This is an internal refactor and does not change any external behavior. Cherry-picked from #895.

codecov · 2020-10-28T18:56:05Z

Codecov Report

Merging #927 into master will decrease coverage by 1.69%.
The diff coverage is 89.47%.

Flag	Coverage Δ
block	`72.74% <89.47%> (-3.73%)`	⬇️
blockchain	`75.34% <89.47%> (-5.24%)`	⬇️
common	`92.03% <ø> (ø)`
ethash	`82.08% <ø> (ø)`
tx	`?`
vm	`87.17% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

ryanio

LGTM, looks really clean!

jochem-brouwer · 2020-10-30T12:47:47Z

I'd like an additional review here before merging this, since it changes the internal stuff a lot, rather have some more eyes on it 😄

@holgerd77 @cgewecke @rumkin

cgewecke

Just to echo @ryanio's review this looks great! Really nice. 🎉

Couple questions, that's it.

cgewecke · 2020-10-30T19:43:39Z

packages/blockchain/src/db/dbManager.ts

+  async batch(ops: DatabaseOperation[]) {
+    const convertedOps: DBOp[] = ops.map((op) => op.baseDBOp)
+    // update the current cache for each operation
+    ops.map((op) => op.updateCache(this._cache))


Is this a substantive change (a bug fix)? (Am just requesting more info - I don't know this lib very well)

This PR does not fix any bugs. Previously, these cache operations were all handled in index.ts of blockchain, but now they are handled within these DatabaseOperations every time we do a batch request of these ops.

packages/blockchain/src/db/databaseOperation.ts

cgewecke

Re-approving 🙂

jochem-brouwer · 2020-10-30T22:01:37Z

I will wait until @holgerd77 also gives this a look (don't worry, not in a hurry). Have to rebase this anyways, so that will need a re-approval anyways 😛

holgerd77 · 2020-11-03T11:09:08Z

Rebased this.

holgerd77 · 2020-11-03T11:18:27Z

I would perceive this as cleaner and less redundant if we remove the DB (or similar) name part from the db folder file names, so:

db/databaseOperation.ts -> db/operation.ts
db/dbConstants -> db/constants.ts
db/dbManager -> db/manager.ts

Prefixes on classes (e.g. class DBManager) or other code structures shouldn't be removed I would say to keep code readability on imports, but we should unify the prefix here as well, there is currently a mixture of DB and Database (DBManager, DatabaseOperation,...). I would have a strong tendency to the shorter DB prefix, so e.g.:

DatabaseOperation -> DBOperation
DatabaseOperationTarget -> DBOperationTarget

And so on.

holgerd77 · 2020-11-03T11:34:44Z

Yeah, after letting the code take a bit its effect I would re-state that I find this renaming thing pretty important, this otherwise leads to very long lines which are somewhat hard to read.

ops.push(DatabaseOperation.del(DatabaseOperationTarget.Header, { blockHash, blockNumber }))

ops.push(DBOps.del(DBTarget.Header, { blockHash, blockNumber }))

Just doing a live-test here on myself with the two lines from above to see how I perceive readability. I think I would actually also tend to do the DatabaseOperationTarget -> DBTarget renaming, think there is not lost anything since these targets very much speak for themselves and this is a lot more comprise.

Apart from these cosmetics this looks really good! Great that this library gets such a shift on readability! 😄

jochem-brouwer · 2020-11-03T16:01:55Z

Hi @holgerd77, thanks for your suggestions! You are absolutely right about the names of these files and the class names, I will edit those. You are also right about the names of these newly introduced helpers, I honestly had a struggle myself to come up with a name for them which was both short and descriptive 😅 .

jochem-brouwer · 2020-11-05T19:40:34Z

OK, in d747941 I have renamed DatabaseOperation -> DBOp, the original exported DBOp type (which is used in level) to DBOperationData and DatabaseOperationTarget to DBTarget. Also renamed the files in the db directory as suggested.

Ready for re-review! @holgerd77 Reviewing d747941 should be sufficient here.

Have to squash here, seems I did something wrong on the rebase.

blockchain: include new DB changes blockchain: lint blockchain: fix tests blockchain: lint

holgerd77 · 2020-11-06T09:14:12Z

Rebased this on top of #929

holgerd77

Have done some more renamings, this looks great, thanks Jochem! 😄 👍

Tx test CI run was failing on last run, tests itself are passing though, this is just a browser timing issue and also highly unlikely to be related with the changes here, since tx has no block or blockchain dependencies.

We might want to observe this a bit, but I will merge here for now.

jochem-brouwer · 2020-11-06T13:02:33Z

Thanks for these extra renames 👍

Tx and Block sometimes indeed seem to timeout; if you rerun the CI then it usually passes.

jochem-brouwer added PR state: needs review type: refactor package: blockchain labels Oct 28, 2020

jochem-brouwer requested review from ryanio and holgerd77 October 28, 2020 18:54

jochem-brouwer mentioned this pull request Oct 28, 2020

Blockchain concurrency and safety improvements #930

Merged

ryanio previously approved these changes Oct 29, 2020

View reviewed changes

cgewecke previously approved these changes Oct 30, 2020

View reviewed changes

jochem-brouwer dismissed stale reviews from cgewecke and ryanio via 07c5f70 October 30, 2020 20:09

cgewecke self-requested a review October 30, 2020 21:09

cgewecke previously approved these changes Oct 30, 2020

View reviewed changes

holgerd77 dismissed cgewecke’s stale review via 9f8ff31 November 3, 2020 11:08

holgerd77 force-pushed the remove-db-coupling-blockchain branch from 07c5f70 to 9f8ff31 Compare November 3, 2020 11:08

jochem-brouwer force-pushed the remove-db-coupling-blockchain branch from 9f8ff31 to d747941 Compare November 5, 2020 19:36

jochem-brouwer force-pushed the remove-db-coupling-blockchain branch from d747941 to a673fdc Compare November 5, 2020 21:14

jochem-brouwer added 3 commits November 6, 2020 10:12

blockchain: refactor DBManager, create databaseOperation factory

0dd6fdb

blockchain: include new DB changes blockchain: lint blockchain: fix tests blockchain: lint

blockchain: import BN from ethereumjs-util

0a1b870

blockchain: rename DB class/types to shorter names

57bcd88

holgerd77 force-pushed the remove-db-coupling-blockchain branch from a673fdc to 57bcd88 Compare November 6, 2020 09:13

blockchain: some more database -> db renamings

35522f8

holgerd77 approved these changes Nov 6, 2020

View reviewed changes

holgerd77 merged commit 5f46d14 into master Nov 6, 2020

holgerd77 deleted the remove-db-coupling-blockchain branch November 6, 2020 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain: refactor: Add abstract DB operation, let DB manage the implementation of this OP + make DB handle Cache #927

Blockchain: refactor: Add abstract DB operation, let DB manage the implementation of this OP + make DB handle Cache #927

jochem-brouwer commented Oct 28, 2020

codecov bot commented Oct 28, 2020 •

edited

ryanio left a comment

jochem-brouwer commented Oct 30, 2020

cgewecke left a comment

cgewecke Oct 30, 2020

jochem-brouwer Oct 30, 2020

cgewecke left a comment

jochem-brouwer commented Oct 30, 2020

holgerd77 commented Nov 3, 2020

holgerd77 commented Nov 3, 2020

holgerd77 commented Nov 3, 2020

jochem-brouwer commented Nov 3, 2020

jochem-brouwer commented Nov 5, 2020 •

edited

holgerd77 commented Nov 6, 2020

holgerd77 left a comment

jochem-brouwer commented Nov 6, 2020

Blockchain: refactor: Add abstract DB operation, let DB manage the implementation of this OP + make DB handle Cache #927

Blockchain: refactor: Add abstract DB operation, let DB manage the implementation of this OP + make DB handle Cache #927

Conversation

jochem-brouwer commented Oct 28, 2020

codecov bot commented Oct 28, 2020 • edited

Codecov Report

ryanio left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Oct 30, 2020

cgewecke left a comment

Choose a reason for hiding this comment

cgewecke Oct 30, 2020

Choose a reason for hiding this comment

jochem-brouwer Oct 30, 2020

Choose a reason for hiding this comment

cgewecke left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Oct 30, 2020

holgerd77 commented Nov 3, 2020

holgerd77 commented Nov 3, 2020

holgerd77 commented Nov 3, 2020

jochem-brouwer commented Nov 3, 2020

jochem-brouwer commented Nov 5, 2020 • edited

holgerd77 commented Nov 6, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Nov 6, 2020

codecov bot commented Oct 28, 2020 •

edited

jochem-brouwer commented Nov 5, 2020 •

edited