HDDS-4840. Make datanode db profile configurable with existing hdds.d…#1955
HDDS-4840. Make datanode db profile configurable with existing hdds.d…#1955elek merged 3 commits intoapache:masterfrom
Conversation
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @guihecheng for working on this. Can you please add some unit tests, eg. in TestKeyValueContainer? (testContainersShareColumnFamilyOptions might be a good place to start.)
There was a problem hiding this comment.
Nit: instead of passing dbProfile member variable, can you please make buildColumnFamilyOptions non-static?
There was a problem hiding this comment.
oh, thanks for your advice, I'll fix it and try to add a simple test case~
guihecheng
force-pushed
the
HDDS-4840
branch
from
14a1cc1 to
e658f40
Compare
|
@adoroszlai updated for review, thanks~ |
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @guihecheng for updating the patch. Mostly looks good to me, but I think TEST profile is unnecessary.
There was a problem hiding this comment.
I think we should try to avoid introducing test-specific items in production code. Why not test with existing DISK and SSD profiles instead?
There was a problem hiding this comment.
@adoroszlai oh, it is simply because the ColumnFamilyOptions for DISK & SSD happen to be the same, as I tested, so a new profile TEST is introduced.
There was a problem hiding this comment.
I don't think they are intended to be the same:
If they are, then it's probably a bug.
There was a problem hiding this comment.
Yes, I've read this line that setting the compactionStyle to CompactionStyle.LEVEL, but actually it is just the default style, so the results are still the same.
There was a problem hiding this comment.
Good point. How about checking non-equality of getDBOptions().compactionReadaheadSize() between the two profiles? Those are different.
There was a problem hiding this comment.
@adoroszlai well, I admit that there merely any difference to check the DBOptions or ColumnFamilyOptions, here I intended to check the ColumnFamilyOptions just because it is convenient to use the existing OPTIONS_CACHE in AbstractDatanodeStore.
So to check the DBOptions, I'll try to export the DBOptions as well(or the DBProfile directly), just for test, what do you think?
There was a problem hiding this comment.
@guihecheng Thanks for checking. I think providing an accessor for AbstractDatanodeStore's DBProfile for the test is much less intrusive than the fake profile.
guihecheng
force-pushed
the
HDDS-4840
branch
2 times, most recently
from
50bdfa9 to
dd5c9ac
Compare
|
@adoroszlai updated the utest case for review, now we get rid of the TEST profile. |
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @guihecheng for updating the patch.
|
@errose28 Would you like to review? |
|
Sure, thanks @adoroszlai and @guihecheng I will take a look. |
errose28
left a comment
errose28
left a comment
There was a problem hiding this comment.
Just a minor inline comment, otherwise LGTM. Thanks @guihecheng for working on this!
There was a problem hiding this comment.
nit: I think we can remove lines 238-239 RocksDB.loadLibrary since this method is no longer static.
There was a problem hiding this comment.
nit: I think we can remove lines 238-239
RocksDB.loadLibrarysince this method is no longer static.
oh, by reading the comment in the code, it seems to be reasonable to remove this line, but I'll check and test it.
There was a problem hiding this comment.
@errose28 It seems that you are right, the RocksDB.loadLibrary is not needed anymore as I testd, will update, thx~
|
well, there's seem to be a unrelated case failure. |
mukul1987
force-pushed
the
master
branch
2 times, most recently
from
79a9d39 to
520ba00
Compare
|
Merging it as we have green build. Thanks the review @adoroszlai and @errose28 |
4 participants
…b.profile
What changes were proposed in this pull request?
Make datanode use the config item
hdds.db.profile, instead of statically default to use spinning disk profile.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4840
Please replace this section with the link to the Apache JIRA)
How was this patch tested?
UT:TestKeyValueContainer:testDBProfileAffectsDBOptions