Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unit tests for database shards #3332

Closed
wants to merge 4 commits into from
Closed

Unit tests for database shards #3332

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 3, 2020

No description provided.

@miguelportilla miguelportilla added the Dependencies Issues associated with 3rd party dependencies (RocksDB, SQLite, etc) label Apr 3, 2020
@codecov-io
Copy link

codecov-io commented Apr 3, 2020 •
edited
Loading

Codecov Report

Merging #3332 into develop will increase coverage by 1.70%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3332      +/-   ##
===========================================
+ Coverage    70.22%   71.93%   +1.70%     
===========================================
  Files          683      683              
  Lines        54621    54624       +3     
===========================================
+ Hits         38360    39293     +933     
+ Misses       16261    15331     -930
Impacted Files Coverage Δ
src/ripple/nodestore/impl/DatabaseShardImp.h 80.00% <ø> (+48.00%) ⬆️
src/ripple/nodestore/impl/DatabaseShardImp.cpp 60.39% <100.00%> (+46.90%) ⬆️
src/ripple/shamap/impl/SHAMap.cpp 90.97% <0.00%> (+0.75%) ⬆️
src/ripple/app/main/Application.cpp 62.24% <0.00%> (+1.06%) ⬆️
src/ripple/shamap/SHAMap.h 100.00% <0.00%> (+3.03%) ⬆️
src/ripple/app/ledger/impl/LedgerToJson.cpp 93.84% <0.00%> (+3.07%) ⬆️
src/ripple/app/ledger/Ledger.cpp 82.29% <0.00%> (+3.29%) ⬆️
src/ripple/basics/TaggedCache.h 80.95% <0.00%> (+3.70%) ⬆️
src/ripple/nodestore/backend/NuDBFactory.cpp 68.98% <0.00%> (+5.69%) ⬆️
src/ripple/nodestore/impl/varint.h 92.68% <0.00%> (+7.31%) ⬆️
... and 10 more

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 023f570...b15afb6. Read the comment docs.

@miguelportilla miguelportilla removed their request for review April 6, 2020 15:48
@miguelportilla miguelportilla removed the Dependencies Issues associated with 3rd party dependencies (RocksDB, SQLite, etc) label Apr 9, 2020
@ghost ghost mentioned this pull request Apr 16, 2020
Closed
HowardHinnant
HowardHinnant previously approved these changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

The new test is now our longest running test by quite a margin. If there is any way to make the test run faster, I'd be in favor of that. However I wouldn't want to do that if it meant compromising the quality of the test. I took a look, and didn't see any obvious way to make the test run faster.

@carlhua
Copy link
Contributor

carlhua commented Apr 22, 2020

Travis CI now passing with updated time out value.

undertome
undertome suggested changes Apr 22, 2020
View reviewed changes
src/ripple/nodestore/impl/DatabaseShardImp.h
@@ -232,7 +232,7 @@ class DatabaseShardImp : public DatabaseShard
std::uint32_t ledgersPerShard_ = ledgersPerShardDefault;

// The earliest shard index
std::uint32_t const earliestShardIndex_;
std::uint32_t earliestShardIndex_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you removed the const qualifier in order to set earliestShardIndex_ in DatabaseShardImp::initConfig. Why is that necessary when this variable gets initialized during the constructor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand why you did this - it's a consequence of the two phase initialization and testing. I'm OK with the change, but I'd add a comment explaining why this is not const.

carlhua
carlhua suggested changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@carlhua carlhua left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the travis.yml changes now the CI is fixed by Travis? Unless there is a reason I think we should not need these changes.

@miguelportilla miguelportilla requested a review from ximinez April 23, 2020 14:28
@ghost
Copy link
Author

ghost commented Apr 23, 2020

Do we need the travis.yml changes now the CI is fixed by Travis? Unless there is a reason I think we should not need these changes.

Done

@ghost
Copy link
Author

ghost commented Apr 23, 2020

Looks good to me.

The new test is now our longest running test by quite a margin. If there is any way to make the test run faster, I'd be in favor of that. However I wouldn't want to do that if it meant compromising the quality of the test. I took a look, and didn't see any obvious way to make the test run faster.

I reduced test time up to 5 times.

HowardHinnant
HowardHinnant reviewed May 15, 2020
View reviewed changes
src/test/nodestore/DatabaseShard_test.cpp Outdated
static constexpr std::uint32_t dataSizeMax = 4;
static constexpr std::uint32_t iniAmount = 1000000;
static constexpr std::uint32_t nTestShards = 4;
static constexpr std::uint32_t shardStoreTimeout = 60;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using the std::chrono timing API: HowardHinnant@d43f168

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@HowardHinnant HowardHinnant self-requested a review May 18, 2020 19:13
HowardHinnant
HowardHinnant suggested changes May 18, 2020
View reviewed changes
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please switch to chrono as recommended for DatabaseShard_test.cpp.

@carlhua carlhua assigned seelabs and unassigned ximinez May 21, 2020
seelabs
seelabs reviewed May 21, 2020
View reviewed changes
.travis.yml Show resolved Hide resolved
seelabs
seelabs reviewed May 21, 2020
View reviewed changes
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let mostly nits. Also agree with @HowardHinnant that chrono would be a good change.

src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
src/test/nodestore/DatabaseShard_test.cpp Outdated Show resolved Hide resolved
@carlhua carlhua self-requested a review May 26, 2020 15:12
@ximinez ximinez removed their request for review May 26, 2020 15:17
HowardHinnant
HowardHinnant approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested 6605e64

@carlhua carlhua requested a review from seelabs May 27, 2020 16:52
seelabs
seelabs approved these changes May 27, 2020
View reviewed changes
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ghost ghost added the Dependencies Issues associated with 3rd party dependencies (RocksDB, SQLite, etc) label May 28, 2020
@ghost
Copy link
Author

ghost commented May 28, 2020

Depends on #3415 .

@carlhua carlhua 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 May 29, 2020
This was referenced May 29, 2020
Closed
Closed
Merged
@miguelportilla
Copy link
Contributor

This PR depends on PR 3415 being merged first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlhua carlhua Awaiting requested review from carlhua

Assignees

@seelabs seelabs

@HowardHinnant HowardHinnant

@undertome undertome

Labels
Dependencies Issues associated with 3rd party dependencies (RocksDB, SQLite, etc) Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@codecov-io @carlhua @miguelportilla @seelabs @HowardHinnant @undertome @ximinez