Adar Dembo 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 poin I'm pining away for protobuf move constructors. (will add comment). 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? I'll tag it advanced. I don't think experimental is appropriate given that modifying it doesn't cause new code paths to execute; it just adjusts how often they run. As for the default value...I don't have a strong opinion, but I'll note that the higher it is, the more metadata write amplification we suffer. That's why I was conservative to start. Did you have a particular value in mind? Line 417: BlockRecordPB* record, > comment on the pointer semantics. it's not clear what's being "saved" here Done PS2, Line 1964: offsets also match > how could the offsets of two live blocks match? It's contrived, but the two blocks could be of zero length. I'll add a comment. PS2, Line 1969: .swap(records); > = std::move(records)? or use .emplace? Done 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 Done Line 2293: continue; > if this were extracted to a new function, this logic would be a bit easier Agreed, done. 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 usag I went back and forth on this. I do like the idea of not making any on-disk changes until we've established that there are no fatal errors, and that's why I ended up on this side of the fence. That said, here's an alternative: if you'll allow me to grow LogBlock by 8 bytes, I can add the block creation timestamp to it, and then I can use the LogBlocks that we've already allocated in the compaction. There's another advantage to this approach: it opens the door to doing compaction at real time without the onerous burden of rereading records from disk. Why didn't I do this already? I couldn't bring myself to grow our memory footprint by potentially dozens of MB without also implementing real time metadata compaction. 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 inte I extracted a function like you suggested, so this has been removed. -- 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 <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
