feat(db/trans-cache): optimize for bloomFilter initialization by halibobo1205 · Pull Request #5394 · tronprotocol/java-tron
| throw new RuntimeException(e); | ||
| } | ||
| try (OutputStream out = new BufferedOutputStream(Files.newOutputStream(file, | ||
| StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE))) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that OS cache flushing to the disk is not immediate, is it possible that the property file flushed successfully but partly of bloomFilters is still in the OS cache(then corrupt when OS crashed)?
Add another option StandardOpenOption.SYNC? Or write all data into one file and make sure the metadata at the end also can achieve the atomic, but this will introduce complexity
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that OS cache flushing to the disk is not immediate, is it possible that the property file flushed successfully but partly of bloomFilters is still in the OS cache(then corrupt when OS crashed)?
Yeah, it's gonna happen. But it doesn't seem to be a problem, it will rollback to previous mode.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add another option StandardOpenOption.SYNC? Or write all data into one file and make sure the metadata at the end also can achieve the atomic, but this will introduce complexity
This PR is a helper feature. StandardOpenOption.SYNC is good , But it may slow down the closing procedure if you're so concerned about closing.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long does StandardOpenOption.SYNC take?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
300ms -> 1.2 s for ssd
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
700ms -> +15s for non-ssd, StandardOpenOption.SYNC is not a good choice!
|
|
||
| private boolean recovery(int index, Path file) { | ||
| try (InputStream in = new BufferedInputStream(Files.newInputStream(file, | ||
| StandardOpenOption.READ, StandardOpenOption.DELETE_ON_CLOSE))) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DELETE_ON_CLOSE means the bloomFilters files only work once, the situation that node recovery finished but shutdown immediately is ignored, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kill -9 or kill -15 ? if kill -15 , bloomFilters will be dump again.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the situation that node recovery finished but shutdown immediately is ignored, right?
Sorry, i've resubmitted it to avoid dump wrong cache, please recheck it.
| private BloomFilter<byte[]>[] bloomFilters = new BloomFilter[2]; | ||
| // filterStartBlock record the start block of the active filter | ||
| private volatile long filterStartBlock = INVALID_BLOCK; | ||
| private volatile long currentBlockNum = INVALID_BLOCK; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A check for currentBlockNum with block header is needed?
Node shutdown using Kill -9 may result in DBS being inconsistent, so this PR only works with a graceful shutdown? Just for a confirm
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A check for currentBlockNum with block header is needed?
Good question, but how might it be inconsistent? Yes ,a graceful shutdown is required
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions, with no relation. A check just for a double check, seems it introduces no more benefit.
how might it be inconsistent
The whole shutdown process is complicated, still thinking
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the empty blocks will make different, a check is not required.
| } | ||
|
|
||
| public void init() { | ||
| if (recovery()) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Recovery failure will affect the initialization logic
- Some exceptions are caught, and some are not. Why is this happening
- Logically speaking, as long as there is an exception, it is a failure, so there is no need to handle those exceptions, so you only need to handle the exception at the outermost layer
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
| "txCache.properties"); | ||
| Map<String, String> properties = readProperties(txCacheProperties); | ||
|
|
||
| if (properties == null || properties.size() != 3) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If properties are read successfully but bloom fails, will there be any problem?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If new fields are added to the properties database in the future, it will have an impact
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If properties are read successfully but bloom fails, will there be any problem?
It will fail and fallback to the original loading mode.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If new fields are added to the properties database in the future, it will have an impact
Wouldn't consider it for now.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
halibobo1205
changed the title
feat(db): optimize for bloomFilter initialization
feat(db/trans-cache): optimize for bloomFilter initialization
This file contains hidden or 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