Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/18569 )

Change subject: KUDU-3371 [fs] Use RocksDB to store LBM metadata
......................................................................


Patch Set 62:

(16 comments)

Thank you for revving the patch!

Overall looks better than a few patchsets ago, but I'm too many down_casts 
bother me.  I'll take a closer look, trying to suggest a way to get rid of 
those.  Meanwhile, maybe you could also take a look at possible options to 
minimize using down_cast?  Using down_cast in many places is a sign that the 
class hierarchy needs to be revisited.

http://gerrit.cloudera.org:8080/#/c/18569/61//COMMIT_MSG
Commit Message:

PS61:
> Yep, you ever asked about it [1].
Right, there are a few main themes behind:
  1. Protect against possible licensing issues (if they change the license of a 
whim)
  2. Reduce the size of RELEASE Kudu binaries.

If taking a brief view on that, it's not clear how to achieve that sort of 
behavior for RocksDB library.  Since RocksDB's API/ABI is C++, not C, dlopen() 
isn't going to work as easy as it would do for C API/ABI.  I was just curious 
whether you've done any further research in that direction.


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc
File src/kudu/fs/dir_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@208
PS66, Line 208: RdbDir::RdbDir(Env* env, DirMetrics* metrics, FsType fs_type, 
std::string dir,
              :                std::unique_ptr<DirInstanceMetadataFile> 
metadata_file,
              :                std::unique_ptr<ThreadPool> pool)
nit for here and elsewhere: in case of a method's signature with many 
parameters spread across multiple lines, consider placing each parameter in its 
own line -- that improves readability and makes it easier to maintain and track 
changes in the long run


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@216
PS66, Line 216: Check 'db_' is only possible to be valid in test environments 
when OpenRocksDB().
nit: try to rephrase this a bit for a better readility


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@217
PS66, Line 217: will reopen
nit: reopen

It's better to use the present tense when describing facts that are true in the 
current version of the code.


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@219
PS66, Line 219: CHECK
nit: does it makes sense to change this to DCHECK, similar to DCHECK_STREQ 
above (since the code paths that could call this method are static, a 
misuse/mistake would be caught in DEBUG builds anyway)?


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@242
PS66, Line 242:   rocksdb::BlockBasedTableOptions tbl_opts;
nit: move this closer to the point of usage


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@268
PS66, Line 268:     return;
nit: is it expected to call Shutdown() multiple times, or that would be a 
programming mistake?  If the latter, consider adding corresponding DCHECK() 
here.


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271
PS66, Line 271: ,
nit: drop the comma


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271
PS66, Line 271: there is no
there aren't any


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271
PS66, Line 271: in flight
in-flight


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@278
PS66, Line 278: ich cause longer time to shut down server.
reduces bootstrapping time upon next start-up.


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@294
PS66, Line 294: }
Is it OK to call this even when 'db_' is null?  If not, consider adding 
corresponding DCHECK() to catch programming mistakes.


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.h@671
PS66, Line 671: };
Please add a small comment describing the essence of this method's 
functionality, and document the parameters as well (similar to the doc for the 
DeleteContainerMetadata method above).


http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@599
PS61, Line 599: The data in RocksDB will be compacted
> When the tserver shut down normally, the RocksDB object will be destoryed i
Thank you for the explanation!


http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@657
PS61, Line 657:
> In fact, I copied this comment from the super class, I will leave the "fata
Ah, so a 'fatal inconsistency' is when there isn't a way to safely 
recover/ignore the error and continue with the rest of server's activity 
without a risk of data corruption.  Thank you for the clarification.


http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.cc@3979
PS66, Line 3979: lockContainerRdbMeta::Open(this, dir,
Maybe, it's possible to introduce a new virtual method into the base class 
called something like Prepare(), and implement it as no-op for Dir::Prepare(), 
while do all the necessary operations there for RdbDir::Prepare()?



--
To view, visit http://gerrit.cloudera.org:8080/18569
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie72f6914eb5653a9c034766c6cd3741a8340711f
Gerrit-Change-Number: 18569
Gerrit-PatchSet: 62
Gerrit-Owner: Yingchun Lai <laiyingc...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <ale...@apache.org>
Gerrit-Reviewer: KeDeng <kdeng...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com>
Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org>
Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com>
Gerrit-Comment-Date: Sat, 06 Jan 2024 04:41:51 +0000
Gerrit-HasComments: Yes

Reply via email to