Todd Lipcon has posted comments on this change. Change subject: KUDU-1549: compact LBM container metadata files at startup ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/fs_report.h File src/kudu/fs/fs_report.h: Line 127: Entry(std::string c, BlockRecordPB* r); this is a little odd -- if it takes a pointer I expect it to store the pointer, but the member is still a value. Add a comment? This is just a hack because protobuf doesn't support move constructor? http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS2, Line 87: DEFINE_double(log_container_live_metadata_before_compact_ratio, 0.10, : "Desired ratio of live block metadata in log containers. If a " : "container's live to total block ratio dips below this value, " : "the container's metadata file will be compacted at startup."); should probably be tagged advanced or even experimental? 0.1 seems like a pretty low value. Isn't the cost relatively low of writing out a new file, considering they are small? Line 417: BlockRecordPB* record, comment on the pointer semantics. it's not clear what's being "saved" here PS2, Line 1964: offsets also match how could the offsets of two live blocks match? PS2, Line 1969: .swap(records); = std::move(records)? or use .emplace? Line 2258: for (const auto& e : low_live_block_containers) { can you extract the body of this if to a new function, perhaps? or too many local variable references? Line 2293: continue; if this were extracted to a new function, this logic would be a bit easier to follow (RETURN_NOT_OK is usable etc) http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: PS2, Line 295: // 3. Containers in 'low_live_block_containers' will have their metadata : // files compacted. : In a typical startup doesn't this mean a fairly large amount of memory usage, vs doing it incrementally after loading each container? I see the appeal of doing all mutative work in one pass at the end, but maybe it's not worth it here, considering these aren't optional "repairs" of unexpected state, but rather just a normal thing we expect would happen on basically every startup? http://gerrit.cloudera.org:8080/#/c/6826/2/src/kudu/util/status.h File src/kudu/util/status.h: Line 60: #define KUDU_WARN_NOT_OK_AND_CONTINUE(to_call, warning_prefix) { \ The naming here is a little confusing, considering 'continue' might be interpreted as 'warn and keep going' rather than 'warn and use the continue keyword to go to the top of the loop'. maybe CONTINUE_LOOP to clarify? or perhaps extracting a functio where this is used could make it so we can use the _AND_RETURN variant instead -- To view, visit http://gerrit.cloudera.org:8080/6826 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I981f7d9e7eb96fb40cef30ad96c5960b72d07756 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
