feat(db/trans-cache): optimize for bloomFilter initialization by halibobo1205 · Pull Request #5394 · tronprotocol/java-tron

halibobo1205 · 2023-08-02T13:54:13Z

codecov-commenter · 2023-08-02T14:23:42Z

Codecov Report

Merging #5394 (eab8d2c) into develop (10223cc) will increase coverage by 0.04%.
The diff coverage is 83.03%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@              Coverage Diff              @@
##             develop    #5394      +/-   ##
=============================================
+ Coverage      60.89%   60.94%   +0.04%     
- Complexity      9231     9246      +15     
=============================================
  Files            839      839              
  Lines          50029    50140     +111     
  Branches        5574     5575       +1     
=============================================
+ Hits           30467    30558      +91     
- Misses         17176    17194      +18     
- Partials        2386     2388       +2

Files Changed	Coverage Δ
.../main/java/org/tron/core/db2/common/TxCacheDB.java	`79.90% <83.03%> (-3.59%)`	⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

halibobo1205 · 2023-08-08T09:00:29Z

@wangzichichi PTAL, https://buildkite.com/tronprotocol/build-on-debian-9-dot-8/builds/4848

tomatoishealthy · 2023-08-09T10:52:03Z

chainbase/src/main/java/org/tron/core/db2/common/TxCacheDB.java

+      throw new RuntimeException(e);
+    }
+    try (OutputStream out = new BufferedOutputStream(Files.newOutputStream(file,
+        StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE))) {


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

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.

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.

How long does StandardOpenOption.SYNC take?

300ms -> 1.2 s for ssd

700ms -> +15s for non-ssd, StandardOpenOption.SYNC is not a good choice!

tomatoishealthy · 2023-08-09T11:15:35Z

chainbase/src/main/java/org/tron/core/db2/common/TxCacheDB.java

+
+  private boolean recovery(int index, Path file) {
+    try (InputStream in = new BufferedInputStream(Files.newInputStream(file,
+        StandardOpenOption.READ, StandardOpenOption.DELETE_ON_CLOSE))) {


DELETE_ON_CLOSE means the bloomFilters files only work once, the situation that node recovery finished but shutdown immediately is ignored, right?

kill -9 or kill -15 ? if kill -15 , bloomFilters will be dump again.

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.

tomatoishealthy · 2023-08-09T11:20:49Z

chainbase/src/main/java/org/tron/core/db2/common/TxCacheDB.java

  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;


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

A check for currentBlockNum with block header is needed?

Good question, but how might it be inconsistent? Yes ,a graceful shutdown is required

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

Maybe the empty blocks will make different, a check is not required.

chainbase/src/main/java/org/tron/core/db2/common/TxCacheDB.java

xxo1shine · 2023-08-14T04:07:53Z

chainbase/src/main/java/org/tron/core/db2/common/TxCacheDB.java

  }

  public void init() {
+    if (recovery()) {


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

xxo1shine · 2023-08-14T04:09:29Z

chainbase/src/main/java/org/tron/core/db2/common/TxCacheDB.java

+        "txCache.properties");
+    Map<String, String>  properties = readProperties(txCacheProperties);
+
+    if (properties == null || properties.size() != 3) {


If properties are read successfully but bloom fails, will there be any problem?

If new fields are added to the properties database in the future, it will have an impact

If properties are read successfully but bloom fails, will there be any problem?

It will fail and fallback to the original loading mode.

If new fields are added to the properties database in the future, it will have an impact

Wouldn't consider it for now.

feat(db): Optimize for bloomFilter initialization

eab8d2c

lvs007 requested review from tomatoishealthy and xxo1shine August 3, 2023 03:46

halibobo1205 added 3 commits August 8, 2023 09:49

fix bloomFilter persistence

080e76c

add test

11c26ba

fix checkStyle

00fac09

halibobo1205 mentioned this pull request Aug 9, 2023

Make the transaction-cache initialization faster #5355

Closed

5 tasks

halibobo1205 added this to the GreatVoyage-v4.7.3 milestone Aug 9, 2023

tomatoishealthy reviewed Aug 9, 2023

View reviewed changes

halibobo1205 self-assigned this Aug 10, 2023

tomatoishealthy approved these changes Aug 14, 2023

View reviewed changes

xxo1shine reviewed Aug 14, 2023

View reviewed changes

halibobo1205 requested a review from jwrct August 14, 2023 11:55

halibobo1205 added 2 commits August 14, 2023 23:59

hide cache-related files

2c6fdac

redesign exception handling

6cd5190

halibobo1205 force-pushed the feat/bloom_flter_init_opt branch from 45840fc to 8566f5a Compare August 14, 2023 17:38

avoid dumping a wrong cached data

7d1eac9

halibobo1205 force-pushed the feat/bloom_flter_init_opt branch from 8566f5a to 7d1eac9 Compare August 14, 2023 17:40

jwrct approved these changes Aug 21, 2023

View reviewed changes

halibobo1205 merged commit f20e11b into tronprotocol:develop Aug 21, 2023

halibobo1205 changed the title ~~feat(db): optimize for bloomFilter initialization~~ feat(db/trans-cache): optimize for bloomFilter initialization Oct 9, 2023

halibobo1205 added type:enhancement minor enhancement area:db labels Oct 19, 2023

Conversation

halibobo1205 commented Aug 2, 2023

Uh oh!

codecov-commenter commented Aug 2, 2023

Codecov Report

Uh oh!

halibobo1205 commented Aug 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants