Fix historical ledger acquisition #3369

mtrippled · 2020-04-21T23:17:36Z

Fix issue where historical ledgers were acquired only since the last
online deletion interval instead of the configured value to allow deletion.

carlhua · 2020-04-22T15:00:21Z

Please update the PR title with something meaningful, please. Thanks!

nbougalis · 2020-04-22T16:45:12Z

Please reword this commit (either here or during a merge) thusly:

Fix historical ledger acquisition when using online deletion:

A previous commit introduced changes in the logic used to acquire historical
ledgers. The logic could cause historical ledgers to be acquired only since the
last online deletion interval instead of the configured value to allow deletion.

The logic change was introduced with e257a22.

mtrippled · 2020-04-22T16:48:26Z

Please update the PR title with something meaningful, please. Thanks!

fixed

mtrippled · 2020-04-22T16:48:44Z

Please reword this commit (either here or during a merge) thusly:

Fix historical ledger acquisition when using online deletion:
A previous commit introduced changes in the logic used to acquire historical
ledgers. The logic could cause historical ledgers to be acquired only since the
last online deletion interval instead of the configured value to allow deletion.
The logic change was introduced with e257a22.

fixed

codecov-io · 2020-04-22T17:37:48Z

Codecov Report

Merging #3369 into develop will increase coverage by 0.00%.
The diff coverage is 73.33%.

@@           Coverage Diff            @@
##           develop    #3369   +/-   ##
========================================
  Coverage    70.41%   70.42%           
========================================
  Files          683      683           
  Lines        54775    54783    +8     
========================================
+ Hits         38570    38579    +9     
+ Misses       16205    16204    -1

Impacted Files	Coverage Δ
src/ripple/app/ledger/LedgerMaster.h	`53.33% <ø> (ø)`
src/ripple/app/misc/SHAMapStore.h	`100.00% <ø> (ø)`
src/ripple/app/misc/SHAMapStoreImp.h	`86.84% <20.00%> (-10.22%)`	⬇️
src/ripple/app/ledger/impl/LedgerMaster.cpp	`42.15% <55.55%> (+0.51%)`	⬆️
src/ripple/app/misc/SHAMapStoreImp.cpp	`75.13% <100.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 020b285...0fff925. Read the comment docs.

scottschurr · 2020-04-22T18:42:51Z

I have the advantage of having visibility of some offline discussions regarding this pull request. One of the points brought up in those discussions was:

There are 4 circumstances that each need different ways to evaulate.

no online delete

online delete with advisory delete

online delete without advisory delete and not currently deleting

online delete without advisory delete and currently deleting

I would like this pull request to incorporate that comment (or something like it) into the code base. And, furthermore, I'd like to see a paragraph for each of those circumstances describing the expected behavior. Then a second paragraph for each circumstance summarizing how the code accomplishes that behavior.

mtrippled · 2020-04-23T22:08:35Z

I have the advantage of having visibility of some offline discussions regarding this pull request. One of the points brought up in those discussions was:

There are 4 circumstances that each need different ways to evaulate.

no online delete

online delete with advisory delete

online delete without advisory delete and not currently deleting

online delete without advisory delete and currently deleting

I would like this pull request to incorporate that comment (or something like it) into the code base. And, furthermore, I'd like to see a paragraph for each of those circumstances describing the expected behavior. Then a second paragraph for each circumstance summarizing how the code accomplishes that behavior.

I cleaned up the conditions for acquiring ledgers, and made a clear comment in the function that determines the minimum ledger to keep.

nbougalis

Looks good. Consider cherry-picking dc7a01b (but double-check if the comment remains accurate).

scottschurr

👍 It looks good to me modulo a few nits. But I didn't see anything I'd hold up the pull request for.

src/ripple/app/ledger/impl/LedgerMaster.cpp

src/ripple/app/misc/SHAMapStore.h

src/ripple/app/ledger/impl/LedgerMaster.cpp

src/ripple/app/misc/SHAMapStore.h

scottschurr · 2020-04-24T20:29:23Z

src/ripple/app/misc/SHAMapStore.h

@@ -66,6 +68,40 @@ class SHAMapStore

    /** Returns the number of file descriptors that are needed. */
    virtual int fdRequired() const = 0;
+
+    /** If online deletion is enabled, return the minimum ledger


Thanks for the comment. It helps.

scottschurr · 2020-04-24T20:35:02Z

src/ripple/app/misc/SHAMapStoreImp.cpp

+        auto const minSql = app_.getLedgerMaster().minSqlSeq();
+        if (minSql.has_value())
+            minimumOnline_ = *minSql;
+    }


Just as a sanity check, if advisorDelete_ is false and there are no local persisted ledgers then minimumOnline_ will be zero. I'm assuming that's the desired behavior?

Good catch--actually, in that case, the desired behavior is to not try to acquire ledgers from the network based on this value. I fixed this in the minimumOnline() function. It would be nice to have a std::atomic<boost::optional> but oh well.

Basically, if we don't know the specific lower bound of things to keep online, don't try to acquire historical ledgers based on this unknown.

src/ripple/app/misc/SHAMapStoreImp.h

scottschurr · 2020-04-24T21:10:37Z

@nbougalis, nice comment formatting. That does improve readability significantly. Thanks.

mtrippled · 2020-04-28T13:50:41Z

Please reword this commit (either here or during a merge) thusly:

Fix historical ledger acquisition when using online deletion:
A previous commit introduced changes in the logic used to acquire historical
ledgers. The logic could cause historical ledgers to be acquired only since the
last online deletion interval instead of the configured value to allow deletion.
The logic change was introduced with e257a22.

I used your commit message (with a slight edit). But I prefer explicitly evaluating has_value() rather than setting the equivalent underlying value because it makes clear that only a known value should trigger an action. The point of optional is to make clear the intentions in that case, and not to rely upon setting and interpreting values for special cases.

nbougalis · 2020-04-28T19:37:06Z

src/ripple/app/misc/SHAMapStore.h

+		the minimum ledger to keep available; an unseated
+                optional otherwise.
+    */
+        virtual boost::optional<LedgerIndex> minimumOnline() const = 0;


Extra spaces here. This can be probably resolved with clang-format better than you manually editing the file. We can do this during merge.

scottschurr

👍 LGTM!

src/ripple/app/misc/SHAMapStoreImp.cpp

src/ripple/app/misc/SHAMapStore.h

src/ripple/app/misc/SHAMapStoreImp.cpp

ximinez

Looks good with the simplifications. I'm still not entirely sold on changing the behavior change with can_delete, but it does work as designed.

scottschurr

Looks good as far as I can see.

scottschurr · 2020-05-19T00:35:50Z

@mtrippled, the commit message of your most recent push looks like it got messed up. You might want to fix that.

mtrippled · 2020-05-19T00:51:04Z

@mtrippled, the commit message of your most recent push looks like it got messed up. You might want to fix that.

Sorry. I thought I'd make the 1st line less than 60 characters, but if it's fine as-is, I reverted it.

scottschurr · 2020-05-19T00:56:57Z

Thanks. Personally, I think a fairly complete sentence for the first line is more important than the 60 character limit. It's all about trade offs....

ximinez

Looks good with the rebase.
Two things of note:

Travis CI failed in the Mac / xcode build. This is an unrelated issue addressed elsewhere, but we should be careful to confirm that the next beta build succeeds.
The formatting of SuiteJournal.h was changed, presumably by the updated version of clang-format that some of us are discussing offline. I don't think any change needs to be made, but we may need to watch out for conflicts later if a bunch of other PRs make the same change.

Commit e257a22 introduced changes in the logic used to acquire historical ledgers. The logic could cause historical ledgers to be acquired only since the last online deletion interval instead of the configured value to allow deletion.

scottschurr

👍

mtrippled requested review from ximinez and scottschurr April 21, 2020 23:17

mtrippled changed the title ~~Fix to e257a226f367b8ad5c706512d39578194c9b7dc2:~~ Fix historical ledger acquisition Apr 22, 2020

mtrippled force-pushed the acquire branch from 112b561 to 0fff925 Compare April 22, 2020 16:47

mtrippled force-pushed the acquire branch from 885bfb1 to 2fafd54 Compare April 23, 2020 22:07

nbougalis previously approved these changes Apr 24, 2020

View reviewed changes

scottschurr previously approved these changes Apr 24, 2020

View reviewed changes

mtrippled dismissed stale reviews from scottschurr and nbougalis via 93f6198 April 28, 2020 13:47

nbougalis previously approved these changes Apr 28, 2020

View reviewed changes

scottschurr assigned ximinez and scottschurr Apr 30, 2020

scottschurr previously approved these changes Apr 30, 2020

View reviewed changes

nbougalis mentioned this pull request May 3, 2020

Proposed 1.6.0-b5 #3388

Merged

ximinez requested changes May 11, 2020

View reviewed changes

mtrippled dismissed stale reviews from scottschurr and nbougalis via c36b1ce May 13, 2020 19:01

carlhua requested review from scottschurr and ximinez May 18, 2020 14:49

ximinez previously approved these changes May 18, 2020

View reviewed changes

scottschurr previously approved these changes May 18, 2020

View reviewed changes

mtrippled dismissed stale reviews from scottschurr and ximinez via 6434036 May 19, 2020 00:29

mtrippled force-pushed the acquire branch from 52a9314 to 6434036 Compare May 19, 2020 00:29

mtrippled force-pushed the acquire branch 2 times, most recently from 736b88a to 5fb1b9e Compare May 19, 2020 00:50

mtrippled added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 19, 2020

ximinez approved these changes May 19, 2020

View reviewed changes

mtrippled force-pushed the acquire branch from 5fb1b9e to 94a67b3 Compare May 21, 2020 21:38

mtrippled force-pushed the acquire branch from 94a67b3 to dac9593 Compare May 21, 2020 21:40

scottschurr approved these changes May 21, 2020

View reviewed changes

manojsdoshi mentioned this pull request May 27, 2020

Proposed 1.6.0-b6 #3413

Merged

manojsdoshi closed this in #3413 May 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix historical ledger acquisition #3369

Fix historical ledger acquisition #3369

mtrippled commented Apr 21, 2020

carlhua commented Apr 22, 2020

nbougalis commented Apr 22, 2020 •

edited

Loading

mtrippled commented Apr 22, 2020

mtrippled commented Apr 22, 2020

codecov-io commented Apr 22, 2020

scottschurr commented Apr 22, 2020

mtrippled commented Apr 23, 2020

nbougalis left a comment

scottschurr left a comment

scottschurr Apr 24, 2020

scottschurr Apr 24, 2020

mtrippled Apr 28, 2020

scottschurr commented Apr 24, 2020

mtrippled commented Apr 28, 2020

nbougalis Apr 28, 2020

scottschurr left a comment

ximinez left a comment

scottschurr left a comment

scottschurr commented May 19, 2020

mtrippled commented May 19, 2020

scottschurr commented May 19, 2020

ximinez left a comment

scottschurr left a comment

Fix historical ledger acquisition #3369

Fix historical ledger acquisition #3369

Conversation

mtrippled commented Apr 21, 2020

carlhua commented Apr 22, 2020

nbougalis commented Apr 22, 2020 • edited Loading

mtrippled commented Apr 22, 2020

mtrippled commented Apr 22, 2020

codecov-io commented Apr 22, 2020

Codecov Report

scottschurr commented Apr 22, 2020

mtrippled commented Apr 23, 2020

nbougalis left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Apr 24, 2020

Choose a reason for hiding this comment

scottschurr Apr 24, 2020

Choose a reason for hiding this comment

mtrippled Apr 28, 2020

Choose a reason for hiding this comment

scottschurr commented Apr 24, 2020

mtrippled commented Apr 28, 2020

nbougalis Apr 28, 2020

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr commented May 19, 2020

mtrippled commented May 19, 2020

scottschurr commented May 19, 2020

ximinez left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

nbougalis commented Apr 22, 2020 •

edited

Loading