refactor: use epochs to gc eth tx hashes from chain indexer #12516
Conversation
|
@akaladarshi Will review this on Monday. Thanks for raising this ! |
|
@akaladarshi Please can you raise this PR against #12521 and change the GC test we already have at |
akaladarshi
changed the base branch from
feat/implement-index-validation-api
to
feat/tests-for-the-chainindexer
akaladarshi
force-pushed
the
akaladarshi/refactor-tx-hash-gc
branch
from
d57f643
to
3495c30
Compare
akaladarshi
force-pushed
the
akaladarshi/refactor-tx-hash-gc
branch
from
3495c30
to
6587808
Compare
chain/index/gc_test.go
Outdated
|
|
||
| si.gc(ctx) | ||
|
|
||
| // tipset at height 1 and 10 should be removed | ||
| // tipset at height 1 data should be removed |
There was a problem hiding this comment.
dont understand this comment. what's "height 1 data" ?
chain/index/gc_test.go
Outdated
| err = si.stmts.getNonRevertedTipsetEventEntriesCountStmt.QueryRow(tsKeyCid2.Bytes()).Scan(&count) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, count) | ||
|
|
||
| // tipset at height 50 should not be removed | ||
| err = si.db.QueryRow("SELECT COUNT(*) FROM tipset_message WHERE height = 50").Scan(&count) |
There was a problem hiding this comment.
lot of duplication here. Can we wrap this and the two sql statements below into a function and re-use it everywhere ?
| return | ||
| } | ||
| currHeadTime := time.Unix(int64(head.MinTimestamp()), 0) | ||
| retentionDuration := time.Duration(si.gcRetentionEpochs*builtin.EpochDurationSeconds) * time.Second |
There was a problem hiding this comment.
Can you add a comment explaining this calculation here ?
Why do we need time.Duration(si.gcRetentionEpochs*builtin.EpochDurationSeconds) * time.Second ?
|
@akaladarshi Some quick comments. |
akaladarshi
force-pushed
the
akaladarshi/refactor-tx-hash-gc
branch
from
4f59281
to
da05e17
Compare
chain/index/gc.go
Outdated
| return | ||
| } | ||
| // Calculate the retention duration based on the number of epochs to retain. | ||
| // retentionDuration represents the total duration (in nano seconds) for which data should be retained before considering it for garbage collection. |
There was a problem hiding this comment.
how is it in nanoseconds ?
There was a problem hiding this comment.
It will be seconds 👍🏾 (I will update it)
so what is happening here is we are converting EpochDurationSeconds(30) to its time duration by multiplying with time.seconds.
There was a problem hiding this comment.
@aarshkshah1992 So time.Duration takes data in nanoseconds (that's why I added nano seconds)
so if I just multiply time.Duration(rententionEpoch * EpochsDurationSeconds), it will give 600 (rententionEpoch = 20, EpochsDurationSeconds=30) but that will be in nanoseconds, to get proper time in seconds we have to multiply with time.Seconds
| totalRetentionDuration := retentionDuration + graceDuration | ||
| currHeadTime := time.Unix(int64(head.MinTimestamp()), 0) | ||
| // gcTime is the time that is (gcRetentionEpochs + graceEpochs) in nano seconds before currHeadTime | ||
| gcTime := currHeadTime.Add(-totalRetentionDuration) |
There was a problem hiding this comment.
if this is less than or equal to 0, return without doing anything. Also please can we had a test for when gcTime <= 0 ?
|
|
||
| log.Infof("gc'ing eth hashes older than %d days", gcRetentionDays) | ||
| res, err = si.stmts.removeEthHashesOlderThanStmt.ExecContext(ctx, "-"+strconv.Itoa(int(gcRetentionDays))+" day") | ||
| res, err = si.stmts.removeEthHashesBeforeTimeStmt.ExecContext(ctx, gcTime.Unix()) |
There was a problem hiding this comment.
why do we need to do .Unix() here ?
There was a problem hiding this comment.
I just wanted to provide as accurate time as possible, so I choose unix.
|
Thanks ! |
aarshkshah1992
merged commit f2bff6f
into
filecoin-project:feat/tests-for-the-chainindexer
Fixes issue: #12465, it changes:
eth_tx_hashquery to delete before epoch instead of daysgcfunction.