HDDS-10107. Remove the static dbNameToCfHandleMap from RocksDatabase. #5976
Conversation
adoroszlai
requested review from
duongkame,
hemantk-12,
sadanand48 and
sumitagrawl
hemantk-12
left a comment
hemantk-12
left a comment
There was a problem hiding this comment.
Thanks @szetszwo for the patch.
Overall looks good to me. Left couple of minor comments.
| this.columnFamilyNames = MemoizedSupplier.valueOf(() -> toMap(columnFamilies.values())); | ||
| } | ||
|
|
||
| private Map<String, ColumnFamily> newMap(List<ColumnFamilyHandle> handles) throws RocksDBException { |
There was a problem hiding this comment.
nit: newMap and toMap are very generic names and seem like can be used to create/convert any type of Map.
I think newMap could be toColumnFamilyMap and toMap be toColumnFamilyIdToNameMap or something along the line.
There was a problem hiding this comment.
Sure, let me rename them.
|
|
||
| @VisibleForTesting | ||
| public ColumnFamilyHandle getHandle() { | ||
| return handle; |
There was a problem hiding this comment.
nit: please remove @VisibleForTesting. it is used in src code now.
There was a problem hiding this comment.
The @VisibleForTesting tag was added by HDDS-8122 since it changed the method from protected to public for a unit test. The getHandle() method is already used many times by in RocksDatabase and a few times in RDBStore.
|
@hemantk-12 , thanks a lot for reviewing this! Just have pushed a commit for addressing your comments. |
2 participants
What changes were proposed in this pull request?
RocksDatabasealready has a non-staticcolumnFamilieswhich hasColumnFamilyHandle. The staticdbNameToCfHandleMapshould be removed for better performance and cleaner code.What is the link to the Apache JIRA
HDDS-10107
How was this patch tested?
By existing tests.