Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17845 )
Change subject: [fs] Refactor fs module ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.h@254 PS1, Line 254: AllDirFailed Rename to AllDirsFailed() or AreAllDirsFailed() http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/dir_manager.cc@227 PS1, Line 227: bool has_healthy_instances = true; Just curious: why to assign any value here if LoadInstances() do not return OK without setting the 'has_healthy_instances' output parameter? http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc File src/kudu/fs/fs_report.cc: http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/fs_report.cc@42 PS1, Line 42: MERGE_CHECKS nit: rename to MERGE_ENTRIES and introduce the 'entries' parameter as well? Otherwise, why to expose only the 'other' parameter out of the macro? http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/fs/log_block_manager.cc@2992 PS1, Line 2992: uint64_t old_metadata_size; nit: if you moved it closer to the site of its usage, maybe move it one more line down right before its usage in the call to env_->GetFileSize()? http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/17845/1/src/kudu/server/server_base.cc@584 PS1, Line 584: LOG(INFO) << s.ToString(); : LOG(INFO) << "This appears to be a new deployment of Kudu; creating new FS layout"; nit: combine these two INFO messages into one? -- To view, visit http://gerrit.cloudera.org:8080/17845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa45637494b36cc1f4f00affdf995a25eb6231bc Gerrit-Change-Number: 17845 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 14 Sep 2021 16:34:14 +0000 Gerrit-HasComments: Yes
