Feature/shard downloader improvements #3252
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3252 +/- ##
===========================================
+ Coverage 70.62% 70.66% +0.04%
===========================================
Files 676 683 +7
Lines 54340 54814 +474
===========================================
+ Hits 38375 38737 +362
- Misses 15965 16077 +112
Continue to review full report at Codecov.
|
undertome
force-pushed
the
feature/shard-downloader-improvements
branch
2 times, most recently
from
05c2943 to
6225b7c
Compare
|
Unit test failure.
|
Yeah, that's an expected log message. As in when there are no failures in that test you'll get a bunch of error logs like that. Currently investigating the Xcode build errors rn though. |
undertome
force-pushed
the
feature/shard-downloader-improvements
branch
3 times, most recently
from
4e4aa83 to
69a1544
Compare
src/ripple/app/main/Application.cpp
Outdated
| auto handler = RPC::ShardArchiveHandler::getInstance( | ||
| *this, | ||
| *m_jobQueue); |
There was a problem hiding this comment.
Why did you convert ShardArchiveHandler into a singleton? Especially since it has a reference to the Application, and is never destroyed once it's created, it seems like it should be owned by the Application and destroyed there.
I see that you're using getInstance in several unit tests, but that can be worked around with something like aShardArchiveHandler& Application::shardArchiveHandler(bool validate) function. Let that function do the logic of figuring out whether there's an instance to create, or just return the existing one.
There was a problem hiding this comment.
With my initial submission it actually was created and destroyed automatically based on the initiation and completion of downloads. With feedback I had to make it a stoppable which means no more destruction until application end.
There was a problem hiding this comment.
If I make the SAH a member of App, then would need synchronization around the construction of the thing. Is it ok to just add a new mutex to the AppImp for that purpose?
There was a problem hiding this comment.
Deferring for now
There was a problem hiding this comment.
There's already a static instance_mutex_ in SAH, so I don't see any harm in moving it to AppImp. (There may be an argument for reusing AppImp::mut_ instead, since that's only used to determine when to stop running and isn't likely to conflict, but that's not a big decision either way.)
|
@miguelportilla Looks like your approval was automatically dismissed after I pushed new changes for Ed's comments? Didn't know about that feature... |
|
Can someone summarize the user-facing changes from this PR for me? There's kind of a lot here for me to jump in and understand how usage or functionality has changed. |
|
@mDuo13 There are no obvious user-facing changes from this PR. It was decided to remove the new RPC commands that were being proposed. It may be worth documenting that with this PR, the RPC shard download command will pause an ongoing download upon the server shutting down and automatically resume that download where it left off on the server startup. |
undertome
force-pushed
the
feature/shard-downloader-improvements
branch
2 times, most recently
from
24c1cf9 to
234d149
Compare
There was a problem hiding this comment.
👍
NOTE: This PR is rebased on PR #3251 which should be merged first.
miguelportilla
added
Dependencies
miguelportilla
dismissed
ximinez’s stale review
Feedback addressed while reviewer on PTO
|
@mDuo13 I just realized, there is a small user facing change. With the download_shard command, the optional |
* Reduce lock scope on all public functions * Use TaskQueue to process shard finalization in separate thread * Store shard last ledger hash and other info in backend * Use temp SQLite DB versus control file when acquiring * Remove boost serialization from cmake files
* Make ShardArchiveHandler a singleton. * Add state database for ShardArchiveHandler. * Use temporary database for SSLHTTPDownloader downloads. * Make ShardArchiveHandler a Stoppable class. * Automatically resume interrupted downloads at server start.
undertome
force-pushed
the
feature/shard-downloader-improvements
branch
from
234d149 to
3780657
Compare
No description provided.