pass unreconciled missing pvtdata to pvtdata store (backport to release-2.2) #1886
Conversation
Signed-off-by: Senthil Nathan N <cendhu@gmail.com>
Signed-off-by: Senthil Nathan N <cendhu@gmail.com> Signed-off-by: David Enyeart <enyeart@us.ibm.com>
Signed-off-by: Senthil Nathan N <cendhu@gmail.com>
| deprioritizedDataReconcilerInterval := 60 * time.Minute | ||
| if viper.IsSet("ledger.pvtdataStore.deprioritizedDataReconcilerInterval") { | ||
| deprioritizedDataReconcilerInterval = viper.GetDuration("ledger.pvtdataStore.deprioritizedDataReconcilerInterval") | ||
| fmt.Println(deprioritizedDataReconcilerInterval) |
| "ledger.pvtdataStore.collElgProcMaxDbBatchSize": 50000, | ||
| "ledger.pvtdataStore.collElgProcDbBatchesInterval": 10000, | ||
| "ledger.pvtdataStore.purgeInterval": 1000, | ||
| "ledger.pvtdataStore.deprioritizedDataReconcilerInterval": "60m", |
There was a problem hiding this comment.
In the test for explicit configs, better to use a value that is different from the default. Else, hard to verify whether the explicit is being picked up or the default one.
| assertMissingDataInfo(t, store, expectedDeprioMissingDataInfo, 2) | ||
|
|
||
| require.True(t, store.accessDeprioMissingDataAfter.After(expectedNextAccessDeprioMissingDataTime)) | ||
| assertMissingDataInfo(t, store, expectedPrioMissingDataInfo, 2) |
There was a problem hiding this comment.
This is dependent on the config value for DeprioritizedDataReconcilerInterval. For this test, it's better to set that explicitly in line 202.
| store.accessDeprioMissingDataAfter = time.Now() | ||
| expectedNextAccessDeprioMissingDataTime := time.Now().Add(store.deprioritizedDataReconcilerInterval) | ||
| assertMissingDataInfo(t, store, expectedDeprioMissingDataInfo, 2) | ||
|
|
||
| require.True(t, store.accessDeprioMissingDataAfter.After(expectedNextAccessDeprioMissingDataTime)) |
There was a problem hiding this comment.
I am not sure that using multiple times time.now could cause this check to be flaky on platforms that provide low resolution time. On the other hand, performing equality check is not possible without introducing an interface. I guess that it will be better to be on a safer side and subtract small duration manually before doing the comparison.
e.g.,
- line 250 can become
store.accessDeprioMissingDataAfter = time.Now().Add(-time.Second)to ensure that line 252 passes. - Reduce time by a second in 251
In general, in this segment of the code, too many time related variable makes it hard to read. Can you see if the variable names can be shortened.
| store.accessDeprioMissingDataAfter = time.Now() | ||
| expectedNextAccessDeprioMissingDataTime := time.Now().Add(store.deprioritizedDataReconcilerInterval) | ||
| assertMissingDataInfo(t, store, expectedDeprioMissingDataInfo, 2) | ||
|
|
||
| require.True(t, store.accessDeprioMissingDataAfter.After(expectedNextAccessDeprioMissingDataTime)) |
There was a problem hiding this comment.
This appears a half check... I guess that better to add a check for the other bound - e.g.,
require.False(t,
store.accessDeprioMissingDataAfter.After(
time.Now().Add(time.Second).Add(store.deprioritizedDataReconcilerInterval))
| // verify missing pvtdata info | ||
| require.Equal(t, uint64(2), blk.Block.Header.Number) | ||
| h.verifyBlockAndPvtDataSameAs(2, blk) | ||
| h.verifyMissingPvtDataSameAs(int(2), expectedMissingPvtDataInfo) |
There was a problem hiding this comment.
This seems redundant, as it was verified in the setup function.
| require.Equal(t, uint64(2), blk.Block.Header.Number) | ||
| h.verifyBlockAndPvtDataSameAs(2, blk) | ||
| h.verifyMissingPvtDataSameAs(int(2), expectedMissingPvtDataInfo) | ||
|
|
|
|
||
| h.commitPvtDataOfOldBlocks(nil, expectedMissingPvtDataInfo) | ||
| for i := 0; i < 5; i++ { | ||
| h.verifyMissingPvtDataSameAs(int(2), ledger.MissingPvtDataInfo{}) |
There was a problem hiding this comment.
why not int(int(2)) :-) at a couple of other places as well.
| # The reconciler would retry deprioritized missing data after every | ||
| # deprioritizedDataReconcilerInterval (unit: minutes). Note that the | ||
| # interval needs to be greater than the reconcileSleepInterval | ||
| deprioritizedDataReconcilerInterval: 60m |
There was a problem hiding this comment.
I would have preferred to keep this config close to reconcileSleepInterval for usability and we could have a check in the code and print a warning if this is relatively smaller than that. However, that would criss-cross in the code unless we make reconciler control this and pass explicitly the category in the call... Per the current code, I am not sure what else can we do, so we can keep this as is.
|
Comments addressed in #1890 (master) and will be backported as well. |
Type of change
Description
Signed-off-by: David Enyeart enyeart@us.ibm.com