wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Jun 19, 2020
Merged
achow101
force-pushed
the
bdb-add-dbbatch
branch
from
b740a6a
to
5fe61d5
Compare
This was referenced Jun 20, 2020
achow101
force-pushed
the
bdb-add-dbbatch
branch
2 times, most recently
from
8c0cc23
to
de11059
Compare
promag
reviewed
Jun 24, 2020
| @@ -161,6 +163,9 @@ class BerkeleyDatabase | |||
| /** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */ | |||
| std::unique_ptr<Db> m_db; | |||
|
|
|||
| /** Make a BerkeleyBatch connected to this database */ | |||
| std::unique_ptr<BerkeleyBatch> MakeBatch(const char* mode, bool flush_on_close); | |||
There was a problem hiding this comment.
I was expecting std::unique_ptr<DatabaseBatch> here - I suppose a future refactor will make BerkeleyDatabase extends WalletDatabase and this method will be virtual?
There was a problem hiding this comment.
Probably wouldn't hurt to do that in this PR. (neh, doing it while adding WalletDatabase in #19334 makes sense)
achow101
force-pushed
the
bdb-add-dbbatch
branch
2 times, most recently
from
d5a1bb0
to
ddd38e0
Compare
|
utACK aec646b |
Instead of having WalletBatch construct the BerkeleyBatch, have BerkeleyDatabase do it and return a std::unique_ptr<BerkeleyBatch>
|
ACK b82f0ca 🌘 Show signature and timestampSignature: Timestamp of file with hash |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 14, 2020
…atabaseBatch abstract class b82f0ca walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow) eac9200 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow) Pull request description: In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`. The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`. Part of bitcoin#18971 Requires bitcoin#19308 and bitcoin#19324 ACKs for top commit: Sjors: re-utACK b82f0ca MarcoFalke: ACK b82f0ca 🌘 meshcollider: LGTM, utACK b82f0ca Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a
maflcko
pushed a commit
that referenced
this pull request
Jul 17, 2020
0cdf2a7 ci: add tsan debug symbols option (Russell Yanofsky) 9a2f126 ci: Add tsan suppression for race in DatabaseBatch (Hennadii Stepanov) Pull request description: Since #19325 was merged, the corresponding change in TSan suppression file gets required. This PR is: - an analogous to #19226 and #19450, and - a temporary workaround for CI fail like https://cirrus-ci.com/task/5741795508224000?command=ci#L4993 ACKs for top commit: MarcoFalke: ACK 0cdf2a7 Tree-SHA512: 7832f143887c8a0df99dea03e00694621710378fbe923e3592185fcd3658546a590693b513abffc5ab96e9ef76c9c4bff3330eeee69a0c5dbe7574f34c417220
meshcollider
added a commit
that referenced
this pull request
Jul 23, 2020
d416ae5 walletdb: Introduce WalletDatabase abstract class (Andrew Chow) 2179dbc walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow) 71d28e7 walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow) 27b2766 walletdb: Move BerkeleyDatabase::Flush(true) to Close() (Andrew Chow) Pull request description: A `WalletDatabase` abstract class is created from `BerkeleyDatabase` and is implemented by `BerkeleyDatabase`. First, to get to the point that this is possible, 4 functions need to be added to `BerkeleyDatabase`: `AddRef`, `RemoveRef`, `Open`, and `Close`. First the increment and decrement of `mapFileUseCount` is refactored into separate functions `AddRef` and `RemoveRef`. `Open` is introduced as a dummy function. This will raise an exception so that it always fails. `Close` is refactored from `Flush`. The `shutdown` argument in `Flush` is removed and instead `Flush(true)` is now the `Close` function. Split from #18971 Requires #19325 ACKs for top commit: ryanofsky: Code review ACK d416ae5. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids fjahr: Code review ACK d416ae5 meshcollider: Code review & test run ACK d416ae5 Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 24, 2020
d416ae5 walletdb: Introduce WalletDatabase abstract class (Andrew Chow) 2179dbc walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow) 71d28e7 walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow) 27b2766 walletdb: Move BerkeleyDatabase::Flush(true) to Close() (Andrew Chow) Pull request description: A `WalletDatabase` abstract class is created from `BerkeleyDatabase` and is implemented by `BerkeleyDatabase`. First, to get to the point that this is possible, 4 functions need to be added to `BerkeleyDatabase`: `AddRef`, `RemoveRef`, `Open`, and `Close`. First the increment and decrement of `mapFileUseCount` is refactored into separate functions `AddRef` and `RemoveRef`. `Open` is introduced as a dummy function. This will raise an exception so that it always fails. `Close` is refactored from `Flush`. The `shutdown` argument in `Flush` is removed and instead `Flush(true)` is now the `Close` function. Split from bitcoin#18971 Requires bitcoin#19325 ACKs for top commit: ryanofsky: Code review ACK d416ae5. Only changes since last review were rebasing after base PR bitcoin#19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids fjahr: Code review ACK d416ae5 meshcollider: Code review & test run ACK d416ae5 Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 1, 2021
Summary: > In order to support alternative database systems, we need to have a generic Batch class. This PR adds a DatabaseBatch abstract class which is implemented by BerkeleyBatch. DatabaseBatch is now the class that is used by WalletBatch to interact with the database. To be able to get the correct type of DatabaseBatch, BerkeleyDatabase now has a MakeBatch function which returns a newly constructed std::unique_ptr<DatabaseBatch>. For BerkeleyDatabase, that will be std::unique_ptr<BerkeleyBatch>. > > The Read, Write, Erase, and Exists template functions are moved from BerkeleyBatch. This is a backport of [[bitcoin/bitcoin#19325 | core#19325]] [1/2] bitcoin/bitcoin@eac9200 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10002
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 1, 2021
Summary: Instead of having WalletBatch construct the BerkeleyBatch, have BerkeleyDatabase do it and return a std::unique_ptr<BerkeleyBatch> This is a backport of [[bitcoin/bitcoin#19325 | core#19325]] [2/2] bitcoin/bitcoin@b82f0ca Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10003
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to support alternative database systems, we need to have a generic
Batchclass. This PR adds aDatabaseBatchabstract class which is implemented byBerkeleyBatch.DatabaseBatchis now the class that is used byWalletBatchto interact with the database. To be able to get the correct type ofDatabaseBatch,BerkeleyDatabasenow has aMakeBatchfunction which returns a newly constructedstd::unique_ptr<DatabaseBatch>. ForBerkeleyDatabase, that will bestd::unique_ptr<BerkeleyBatch>.The
Read,Write,Erase, andExiststemplate functions are moved fromBerkeleyBatch.Part of #18971
Requires #19308 and #19324