Feature/shard path multiplexing #3463

undertome · 2020-06-27T15:37:58Z

This PR introduces changes that allow a user to specify multiple paths for storing historical shards. The changes are based in the DatabaseShardImp class and in the shards unit tests. Please read the documentation to get an idea of the changes involved.

src/ripple/nodestore/ShardStoragePaths.md

ghost

I compiled, test run, and studied part of code related to Deterministic shards. Looks good.

seelabs

Nice job on this! I left some comments to consider.

src/ripple/nodestore/ShardStoragePaths.md

src/ripple/nodestore/impl/DatabaseShardImp.cpp

src/ripple/nodestore/impl/DeterministicShard.h

src/ripple/nodestore/impl/Shard.h

src/ripple/rpc/impl/ShardArchiveHandler.cpp

src/test/nodestore/DatabaseShard_test.cpp

src/ripple/nodestore/ShardStoragePaths.md

src/ripple/net/ShardDownloader.md

src/ripple/nodestore/impl/DatabaseShardImp.cpp

src/ripple/nodestore/impl/DatabaseShardImp.h

src/ripple/rpc/impl/ShardArchiveHandler.cpp

src/ripple/nodestore/impl/DatabaseShardImp.cpp

ximinez · 2020-08-11T22:04:31Z

@undertome In the future, please do not rewrite history and force push a single commit over and over. Instead, add additional commits onto your branch, starting the commit message with "[FOLD]" to indicate that these commits will need to be squashed once the PR is passed or merged. This makes my job as reviewer exponentially more manageable for several reasons, including I can confirm that you addressed all issues, I can see precisely what changed since the last time I looked at the PR, and I can focus on those changes and lower the risk of missing something that gets "buried" in the original change set. Thanks!

src/ripple/nodestore/impl/DatabaseShardImp.cpp

undertome · 2020-08-12T13:47:53Z

@undertome In the future, please do not rewrite history and force push a single commit over and over. Instead, add additional commits onto your branch, starting the commit message with "[FOLD]" to indicate that these commits will need to be squashed once the PR is passed or merged. This makes my job as reviewer exponentially more manageable for several reasons, including I can confirm that you addressed all issues, I can see precisely what changed since the last time I looked at the PR, and I can focus on those changes and lower the risk of missing something that gets "buried" in the original change set. Thanks!

Sure thing!

seelabs

Left a few comments for consideration.

src/ripple/net/ShardDownloader.md

src/ripple/nodestore/ShardStoragePaths.md

src/ripple/nodestore/impl/DatabaseShardImp.h

src/ripple/nodestore/impl/DatabaseShardImp.cpp

seelabs

Left some additional comments, but nothing that should hold this up (although the clang format issue does need to be resolved). 👍 Great job on this!

src/ripple/nodestore/impl/DatabaseShardImp.cpp

src/ripple/nodestore/impl/DatabaseShardImp.h

src/ripple/nodestore/impl/DatabaseShardImp.cpp

src/ripple/nodestore/impl/DatabaseShardImp.h

ximinez

One oversight, but otherwise, I think I'm good.

seelabs

Looks like there's an issue with the VS build:

(WarnCompileDuplicatedFilename target) -> 

         C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\Common7\IDE\VC\VCTargets\Microsoft.CppBuild.targets(944,5): warning MSB8027: Two or more files with the name of Manifest.cpp will produce outputs to the same location. This can lead to an incorrect build result.  The files involved are C:\Users\travis\build\ripple\rippled\src\ripple\app\misc\impl\Manifest.cpp, C:\Users\travis\build\ripple\rippled\src\ripple\rpc\handlers\Manifest.cpp. [C:\Users\travis\build\ripple\rippled\build.ms\rippled.vcxproj]

We've run into similar issues in the past and have solved it by renaming one of the files.

undertome · 2020-08-19T16:06:22Z

Looks like there's an issue with the VS build:

(WarnCompileDuplicatedFilename target) -> 

         C:\Program Files (x86)\Microsoft Visual Studio\2017\BuildTools\Common7\IDE\VC\VCTargets\Microsoft.CppBuild.targets(944,5): warning MSB8027: Two or more files with the name of Manifest.cpp will produce outputs to the same location. This can lead to an incorrect build result.  The files involved are C:\Users\travis\build\ripple\rippled\src\ripple\app\misc\impl\Manifest.cpp, C:\Users\travis\build\ripple\rippled\src\ripple\rpc\handlers\Manifest.cpp. [C:\Users\travis\build\ripple\rippled\build.ms\rippled.vcxproj]

We've run into similar issues in the past and have solved it by renaming one of the files.

Let's address this in a separate PR since this one didn't change those files.

ximinez · 2020-08-19T16:14:10Z

@seelabs I think that error is a non-fatal warning, and the actual CI failure was due to Travis being Travis. When I rebuilt that step, it succeeded. https://travis-ci.com/github/ripple/rippled/jobs/375010094

seelabs

👍 Remarking this as approved. The duplicate file name issue was not created in this patch.

* Distinguish between recent and historical shards * Allow multiple storage paths for historical shards * Add documentation for this feature * Add unit tests

miguelportilla · 2020-08-13T14:22:30Z

src/ripple/nodestore/impl/DatabaseShardImp.cpp

            // Get the available storage for each historical path
            std::for_each(
                historicalPaths_.begin(),
                historicalPaths_.end(),
                [&capacities](boost::filesystem::path const& path) {
-                    capacities.push_back(
-                        boost::filesystem::space(path).available);
+


Compile error, uniqueCapacities isn't captured on non Linux.

miguelportilla · 2020-08-13T14:24:43Z

src/ripple/nodestore/impl/DatabaseShardImp.cpp

+                        uniqueCapacities.end())
+                    {
+                        uniqueCapacities[availableSpace].push_back(
+                            path.to_string());


Compile error, to_string() isn't a class function, use string().

miguelportilla · 2020-08-13T14:24:49Z

src/ripple/nodestore/impl/DatabaseShardImp.cpp

+                            path.to_string());
+                    }
+                    else
+                        uniqueCapacities[availableSpace] = {path.to_string()};


Compile error, to_string() isn't a class function, use string().

undertome requested a review from miguelportilla June 27, 2020 15:37

carlhua assigned miguelportilla Jun 27, 2020

miguelportilla requested a review from ximinez June 29, 2020 17:11

miguelportilla assigned ximinez Jun 29, 2020

miguelportilla requested a review from thejohnfreeman June 29, 2020 17:16

miguelportilla assigned thejohnfreeman and unassigned miguelportilla Jun 29, 2020

miguelportilla requested a review from a user June 29, 2020 17:19

miguelportilla assigned ghost Jun 29, 2020

miguelportilla removed the request for review from thejohnfreeman June 29, 2020 17:21

miguelportilla unassigned thejohnfreeman Jun 29, 2020

MarkusTeufelberger reviewed Jun 29, 2020

View reviewed changes

src/ripple/nodestore/ShardStoragePaths.md Outdated Show resolved Hide resolved

MarkusTeufelberger reviewed Jun 29, 2020

View reviewed changes

src/ripple/nodestore/ShardStoragePaths.md Outdated Show resolved Hide resolved

undertome force-pushed the feature/shard-path-multiplexing branch from be33a9b to b70ad9d Compare July 3, 2020 22:43

undertome mentioned this pull request Jul 10, 2020

Doc consolidation #3519

Closed

ghost reviewed Jul 10, 2020

View reviewed changes

carlhua assigned cjcobb23 and seelabs and unassigned ximinez Jul 17, 2020

undertome requested a review from seelabs July 21, 2020 17:57

seelabs reviewed Jul 21, 2020

View reviewed changes

undertome force-pushed the feature/shard-path-multiplexing branch from 4b5a6c0 to 5bb6a64 Compare July 28, 2020 22:16

miguelportilla reviewed Jul 29, 2020

View reviewed changes

src/test/nodestore/DatabaseShard_test.cpp Show resolved Hide resolved

undertome force-pushed the feature/shard-path-multiplexing branch from 2625285 to fce3310 Compare July 29, 2020 22:15

ximinez requested changes Jul 30, 2020

View reviewed changes

undertome force-pushed the feature/shard-path-multiplexing branch 3 times, most recently from 56804d9 to 157ed88 Compare August 11, 2020 16:07

ximinez reviewed Aug 11, 2020

View reviewed changes

seelabs reviewed Aug 12, 2020

View reviewed changes

seelabs approved these changes Aug 13, 2020

View reviewed changes

src/ripple/nodestore/impl/DatabaseShardImp.cpp Outdated Show resolved Hide resolved

src/ripple/nodestore/impl/DatabaseShardImp.cpp Outdated Show resolved Hide resolved

src/ripple/nodestore/impl/DatabaseShardImp.cpp Outdated Show resolved Hide resolved

undertome force-pushed the feature/shard-path-multiplexing branch from 6559199 to 37b7d29 Compare August 13, 2020 16:19

carlhua assigned ximinez and unassigned cjcobb23 Aug 17, 2020

ximinez requested changes Aug 17, 2020

View reviewed changes

ximinez reviewed Aug 18, 2020

View reviewed changes

src/ripple/nodestore/impl/DatabaseShardImp.cpp Show resolved Hide resolved

src/ripple/nodestore/impl/DatabaseShardImp.h Show resolved Hide resolved

ximinez requested changes Aug 18, 2020

View reviewed changes

ximinez approved these changes Aug 19, 2020

View reviewed changes

seelabs suggested changes Aug 19, 2020

View reviewed changes

seelabs approved these changes Aug 19, 2020

View reviewed changes

undertome added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Aug 19, 2020

Allow multiple paths for shard storage:

c5d7214

* Distinguish between recent and historical shards * Allow multiple storage paths for historical shards * Add documentation for this feature * Add unit tests

undertome force-pushed the feature/shard-path-multiplexing branch from b834b56 to c5d7214 Compare August 19, 2020 16:40

nbougalis mentioned this pull request Sep 1, 2020

Proposed 1.7.0-b1 #3599

Merged

nbougalis closed this in #3599 Sep 8, 2020

miguelportilla reviewed Sep 11, 2020

View reviewed changes

mDuo13 mentioned this pull request Nov 3, 2020

Document Multiple shard paths XRPLF/xrpl-dev-portal#923

Closed

mDuo13 mentioned this pull request Mar 1, 2021

Shard multiplexing docs XRPLF/xrpl-dev-portal#979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/shard path multiplexing #3463

Feature/shard path multiplexing #3463

undertome commented Jun 27, 2020 •

edited

Loading

ghost left a comment

seelabs left a comment

ximinez commented Aug 11, 2020

undertome commented Aug 12, 2020

seelabs left a comment

seelabs left a comment

ximinez left a comment

seelabs left a comment

undertome commented Aug 19, 2020

ximinez commented Aug 19, 2020

seelabs left a comment

miguelportilla Aug 13, 2020

miguelportilla Aug 13, 2020

miguelportilla Aug 13, 2020

Feature/shard path multiplexing #3463

Feature/shard path multiplexing #3463

Conversation

undertome commented Jun 27, 2020 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

ximinez commented Aug 11, 2020

undertome commented Aug 12, 2020

seelabs left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

undertome commented Aug 19, 2020

ximinez commented Aug 19, 2020

seelabs left a comment

Choose a reason for hiding this comment

miguelportilla Aug 13, 2020

Choose a reason for hiding this comment

miguelportilla Aug 13, 2020

Choose a reason for hiding this comment

miguelportilla Aug 13, 2020

Choose a reason for hiding this comment

undertome commented Jun 27, 2020 •

edited

Loading