Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15830 )
Change subject: [fs] fixed file num low live ratio metadata compaction policy when startup ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@7 PS4, Line 7: [fs] fixed file num low live ratio metadata compaction policy when : startup nit: keep the title on a single line http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@10 PS4, Line 10: it loads all low live ratio metadata into memory, nit: I wasn't sure what "low live ratio metadata" referred to. Could you also add a small description here, e.g. "it loads all low live ratio metadata into memory (i.e. metadata for containers that contain a low number of live blocks), and this is..." http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@11 PS4, Line 11: controll nit: "control" with one 'l' http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@16 PS4, Line 16: excess nit: "exceed" http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@18 PS4, Line 18: livemetadata nit: add a space: "live metadata" http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@29 PS4, Line 29: rting(Not nit: add a space in between "starting (", and "Not" should be lower cased. http://gerrit.cloudera.org:8080/#/c/15830/4//COMMIT_MSG@30 PS4, Line 30: excess nit: "exceeds" http://gerrit.cloudera.org:8080/#/c/15830/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/15830/4/src/kudu/fs/log_block_manager.cc@119 PS4, Line 119: DEFINE_int32(max_num_low_live_ratio_metadata_file_to_compact, -1, nit: can you put this up by log_container_live_metadata_before_compact_ratio so the compaction-related flags are adjacent to one another? http://gerrit.cloudera.org:8080/#/c/15830/4/src/kudu/fs/log_block_manager.cc@120 PS4, Line 120: nit: spell out "number" http://gerrit.cloudera.org:8080/#/c/15830/4/src/kudu/fs/log_block_manager.cc@2668 PS4, Line 2668: low_live_ratio_metadata_to_compact_num_.Increment(); Using the number of metadata files seems kind of odd, given it requires some experimentation or knowledge beforehand about what is being stored in memory to use it effectively. Would it be possible to measure size via BlockRecordPB::ByteSizeLong() and limit the number of bytes we're allowed to collect for compaction instead? As far as I can tell, ByteSizeLong() is the post-encoding size, so it may not be accurate. But if we could get the in-memory size of the records, we could apply some bytes limit, which seems more natural. -- To view, visit http://gerrit.cloudera.org:8080/15830 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifad1d6de4e61a1ddcfb35743abd71b57d6418896 Gerrit-Change-Number: 15830 Gerrit-PatchSet: 4 Gerrit-Owner: wangning <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: wangning <[email protected]> Gerrit-Comment-Date: Thu, 14 May 2020 03:51:36 +0000 Gerrit-HasComments: Yes
