Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21570 )
Change subject: KUDU-3371 Parameterize some options of RocksDB ...................................................................... Patch Set 1: (26 comments) http://gerrit.cloudera.org:8080/#/c/21570/1//COMMIT_MSG Commit Message: PS1: I guess this patch should be rebased on top of https://gerrit.cloudera.org/#/c/21560/ (or vice versa) and the newly introduced flags be put under corresponding ifdefs, otherwise it will be necessary to add one more patch in addition to these already existing ones. http://gerrit.cloudera.org:8080/#/c/21570/1//COMMIT_MSG@10 PS1, Line 10: some : necessary options Just curious: how did you find those are 'necessary' ones among many? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/cfile/block_cache.cc@119 PS1, Line 119: ($0 MB vs. $1 MB). Substitution parameters are missing -- this might crash. http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/cfile/block_cache.cc@120 PS1, Line 120: performance problems I'm curious: what sort of performance issues might be there in such a case and why? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/cfile/block_cache.cc@123 PS1, Line 123: return false If it's just a warning (at least it seems so due to LOG(WARNING) above), this should return 'true'. Otherwise, change LOG(WARNING) to LOG(ERROR). BTW, why this should be an error, not just a warning about a mis-configuration or a typo? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@76 PS1, Line 76: more details for details http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@76 PS1, Line 76: Average bits Average number of bits http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@79 PS1, Line 79: TAG_FLAG(log_container_rdb_bits_per_key, experimental); Shouldn't all these new flags be tagged as 'experimental'? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@82 PS1, Line 82: uint64 Would uint32 suffice here? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@83 PS1, Line 83: MB MiB? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@87 PS1, Line 87: uint64 Would uint32 suffice here? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@88 PS1, Line 88: concurrent shared background jobs concurrent background jobs http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@89 PS1, Line 89: between shared between http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@92 PS1, Line 92: DEFINE_uint64 Would uint32 suffice here? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@107 PS1, Line 107: Amount The amount http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@107 PS1, Line 107: the nit: drop 'the' http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@113 PS1, Line 113: will be are http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@114 PS1, Line 114: dirs dir http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@114 PS1, Line 114: non empty nit: 'not empty' or 'non-empty' http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@120 PS1, Line 120: Specify the maximal size Maximum size http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@121 PS1, Line 121: it specified http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@125 PS1, Line 125: Maximal Maximum number of ... http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@125 PS1, Line 125: to be kept to keep http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@129 PS1, Line 129: limit What are the units of the limit? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@133 PS1, Line 133: DEFINE_int64 Would int32 suffice here? http://gerrit.cloudera.org:8080/#/c/21570/1/src/kudu/fs/dir_manager.cc@135 PS1, Line 135: number the number -- To view, visit http://gerrit.cloudera.org:8080/21570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6a6da6868511fe69af528ca122fcaa98cfc9dac4 Gerrit-Change-Number: 21570 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 12 Jul 2024 00:52:13 +0000 Gerrit-HasComments: Yes
