Home Original page

feat(db): optimize old rewards withdrawal by halibobo1205 · Pull Request #5406 · tronprotocol/java-tron

@halibobo1205

@halibobo1205

tomatoishealthy

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these removals part of the optimization reward calculation? If not, better fix it in another PR, just suggestion

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a class conflict.

tomatoishealthy

Metrics.histogramObserve(requestTimer);
}

private Protocol.Account getAccount(byte[] address) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get the account using accountStore.getFromRoot() directly？

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB Cache pollution due to bulk queries, like accountStore.getFromRoot()

tomatoishealthy

default UnmodifiableIterator<Entry<byte[], byte[]>> prefixQueryAfterThat
(byte[] key, byte[] afterThat) {
this.seek(afterThat == null ? key : afterThat);
return Iterators.filter(this, entry -> Bytes.indexOf(entry.getKey(), key) == 0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I have doubt, whether this iterator will still access all data in the database but just return the data that matched the filter.
If so, it will do a lot of useless read-op?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not All, iterates over all data after the prefix. I can't think of a better way for now. It takes 2 minutes to iterate the database.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the default iterator and move the filter logic into startRewardCal, or re-implement a prefix iterator.
Seems 2 minutes is not a big deal, depends on you

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix does not work is fixed.

@halibobo1205 

   System.out.println((long) 128.9999999999999857891452847979962825775146484375); // will print 129
   System.out.println((long) 128.9999999999999857891452847979962825775146484374999999999999999999); // will print 128

tomatoishealthy

jwrct

xxo1shine
private long getOldReward(long beginCycle, long oldEndCycle, AccountCapsule accountCapsule) {
long cacheData = rewardCalService.getReward(accountCapsule.createDbKey(), beginCycle);
long reward = 0;

This comment was marked as resolved.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phase I only calculates without using, cache enabled for the next release.

return null;
}

long computeReward(long cycle, Protocol.Account account) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why rewrite this method instead of calling the original method?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid circular dependencies.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@codecov-commenter

tomatoishealthy

@@ -13,23 +14,22 @@ public final class StoreIterator implements org.tron.core.db.common.iterator.DBI
private final DBIterator dbIterator;
private boolean first = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a simpler way by just setting first false when invoking seek() & seekToLast() to solve the problem of pointer confusion?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly how it's handled.

xxo1shine

tomatoishealthy

jwrct

jwrct

}

public boolean useNewRewardAlgorithmFromStart() {
return getNewRewardAlgorithmEffectiveCycle() == 1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this value possibly equal 1, even on a new chain?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, It can be turned on anytime.

@halibobo1205

@halibobo1205

jwrct

try {
lock.await();
} catch (InterruptedException e) {
logger.error("rewardViCalService lock error: {}", e.getMessage());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an InterruptedException occurs, the program will continue to execute, but with an additional error log. Is this the expected behavior?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@halibobo1205

@halibobo1205

tomatoishealthy

this.clearUp(true);
logger.info("rewardViCalService is already done");
} else {
if (this.getLatestBlockHeaderNumber() > lastBlockNumber) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is getLatestSolidifiedBlockNum & lastSolidifiedBlockNumber better?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This service is depend on root dbs.

yanghang8612

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@halibobo1205

Review task on 2.28: