Refactor and test journal pruning class #482
Conversation
AlexandraRoatis
requested review from
qoire,
iamyulong,
AionJayT and
aion-sridevi
| if (!enabled) { | ||
| return; | ||
| } | ||
| ByteArrayWrapper hash = new ByteArrayWrapper(blockHash); |
There was a problem hiding this comment.
use ByteArrayWrapper.wrap() intead of new ByteArrayWrapper()
| try { | ||
| if (!enabled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
should we check enable before the writelock?
| enabled = e; | ||
|
|
There was a problem hiding this comment.
use atomicboolean for the variable - enabled, then don't need lock in this method also check enabled before the other methods before writelock.
| return src.get(key); | ||
| } catch (Exception e) { | ||
| throw e; | ||
| } finally { |
There was a problem hiding this comment.
for the following throws(including this) , add an error log before throw if no outside classed catch these exceptions.
AlexandraRoatis
force-pushed
the
db-prune-refactor
branch
from
99d9b24
to
527f382
Compare
|
rebased the branch to include changed to #478 |
|
General question about the reference counts: Is it possible to remove the DB read that happens when we initialize a ref in We only seem to use it in removing fork blocks, so I believe we can do a check if absolutely necessary in that scenario but otherwise we can ignore it. |
|
Looking into the above refactoring I discovered a bug relating to the reference count. Working on it now. |
|
Corrected the bug. Refactoring the reference to check for presence in the database only at rollback does not work because the pruning can no longer distinguish between inserts to be reverted (on side chain) and inserts to be kept (main chain). Check behavior for block b2 key k5 in Note: If you want, the refactoring can be done with the caveat that keys like the ones above will be kept in the database despite having outlived their utility. This means trading db access at run time for space taken up by stray keys that could not be deleted due to the missing functionality. |
|
LGTM |
Description
Please include a brief summary of the change that this pull request proposes. Include any relevant motivation and context. List any dependencies required for this change.
JournalPruneDataSource.javaindependent of the block and block header classesstateDatabasesince the only class accessing it is theJournalPruneDataSourceRelated to issues #54 and #298.
Note: this PR builds on top of #478.
The PR in #478 should be reviewed and merged first. After which this PR can be rebased to
master-pre-mergebranch.Type of change
Insert x into the following checkboxes to confirm (eg. [x]):
Testing
Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.
Verification
Insert x into the following checkboxes to confirm (eg. [x]):