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.

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

feat(rocksdb): configure rocksdb options #1065

Merged

Conversation

moreal
Copy link
Contributor

@moreal moreal commented Nov 2, 2020

It provides a new feature to configure some options of RocksDB options.

With maxTotalWalSize, you can limit the size of WAL.
With keepLogFileNum, you can limit the number of log files.

public RocksDBStore(
string path,
int blockCacheSize = 512,
int txCacheSize = 1024,
int statesCacheSize = 10000
int statesCacheSize = 10000,
ulong? maxTotalWalSize = null,
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should use long instead of ulong on public interface to keep CLS compatibility.

Copy link
Contributor Author

@moreal moreal Nov 2, 2020

Choose a reason for hiding this comment

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

I used its type as ulong because the type of DbOptions.SetKeepLogFileNum()'s argument was ulong. I will amend it to keep CLS compatibility.

I tried it but I couldn't convert long to ulong 😢

Copy link
Member

Choose a reason for hiding this comment

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

I guess explicit cast((ulong)) will work...

long maxTotalWalSize = 0L;
 _options.SetMaxTotalWalSize((ulong)maxTotalWalSize);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it is okay to use ulong type for convert to keep CLS compatibility. I guess the ulong shouldn't appear anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

As far I know, CLS compliance apply only to public interface, not to its private implementation. (see also: https://docs.microsoft.com/en-US/dotnet/standard/language-independence-and-language-independent-components#Rules)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about giving up CLS compatibility only for Libplanet.RocksDBStore project? Although we can cast long to ulong, we still lost its upper bits (9,223,372,036,854,775,808 to 18,446,744,073,709,551,615).

Copy link
Member

Choose a reason for hiding this comment

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

If we need higher value for these options, I agree with giving up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I checked the RocksDB C++ core also used the option's type as uint64 (ulong). So I will give up it and stay with this code. @longfin

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #1065 into 9c-main will decrease coverage by 0.08%.
The diff coverage is 70.37%.

@@             Coverage Diff             @@
##           9c-main    #1065      +/-   ##
===========================================
- Coverage    88.39%   88.30%   -0.09%     
===========================================
  Files          343      343              
  Lines        31509    31520      +11     
===========================================
- Hits         27853    27835      -18     
- Misses        1975     2007      +32     
+ Partials      1681     1678       -3     
Impacted Files Coverage Δ
Libplanet.RocksDBStore/RocksDBStore.cs 94.70% <27.27%> (-1.19%) ⬇️
Libplanet/Net/BlockDemand.cs 100.00% <100.00%> (ø)
Libplanet/Net/Swarm.MessageHandlers.cs 84.83% <100.00%> (ø)
Libplanet/Net/Swarm.cs 83.33% <100.00%> (-1.42%) ⬇️
Libplanet/Store/DefaultStore.cs 85.75% <0.00%> (-0.95%) ⬇️
Libplanet.Tests/Net/SwarmTest.Preload.cs 98.52% <0.00%> (-0.33%) ⬇️
Libplanet/Net/NetMQTransport.cs 81.82% <0.00%> (+0.12%) ⬆️
Libplanet/Net/Protocols/KademliaProtocol.cs 80.29% <0.00%> (+0.84%) ⬆️
... and 1 more

limebell
limebell previously approved these changes Nov 3, 2020
@moreal moreal requested a review from longfin November 3, 2020 06:33
longfin
longfin previously approved these changes Nov 3, 2020
@moreal moreal dismissed stale reviews from longfin and limebell via f13a8bf November 3, 2020 06:33
@moreal moreal force-pushed the feature/rocksdb/configure-options branch from 97a249b to f13a8bf Compare November 3, 2020 06:33
earlbread
earlbread previously approved these changes Nov 3, 2020
limebell
limebell previously approved these changes Nov 3, 2020
@moreal moreal dismissed stale reviews from limebell and earlbread via ba53342 November 3, 2020 07:29
@moreal moreal force-pushed the feature/rocksdb/configure-options branch from f13a8bf to ba53342 Compare November 3, 2020 07:29
earlbread
earlbread previously approved these changes Nov 3, 2020
@moreal
Copy link
Contributor Author

moreal commented Nov 3, 2020

@planetarium/libplanet Could you review this pull request?

@longfin longfin merged commit ed381f0 into planetarium:9c-main Nov 3, 2020
@moreal moreal mentioned this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants