Adar Dembo has posted comments on this change. Change subject: log block manager: detect and repair unpunched holes ......................................................................
Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/6717/5/src/kudu/fs/log_block_manager-test-util.cc File src/kudu/fs/log_block_manager-test-util.cc: PS5, Line 156: initial_data_size > does it make sense to add assertions on size? Not sure what you mean; do you mean we should assert that initial_data_size is larger or equal to a certain value? What value would that add? PS5, Line 460: int64_t* old_data_file_size > maybe set a nullptr default arg so that you don't have to pass it if you do All three callers make use of old_data_file_size though. http://gerrit.cloudera.org:8080/#/c/6717/5/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 50: > extra line, likely also clean the other one below Done http://gerrit.cloudera.org:8080/#/c/6717/5/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS5, Line 79: 0.10 > this is what's gating the heavy startup penalty, right? How easy is it to r Will change the name. If the penalty you're alluding to was the 50% from the extent map approach, that's totally gone. Without any slop (meaning, any full container with even one unaccounted-for byte will repunch its holes and truncate), I observed a 6% penalty on el6.6 where my ~10,000 containers of size 16-32k had about 2k extra space in them. We still don't really know why these discrepancies exist. Todd's theory is that older versions of ext4 include blocks occupied by interior nodes in the extent tree in their accounting, but I wasn't able to find a slam dunk explanation in a bug report or similar resource. And I don't have time to do a thorough investigation in different environments. So, this knob will have to suffice. Note: I didn't see any space discrepancies at all in Ubuntu 16; that is, at some point, ext4 fixed its space accounting issues. Just not on el6. PS5, Line 80: Additional fraction of a log container's calculated size that " : "must be consumed on disk before the container considered to be " : "inconsistent and subject to excess space cleanup. > explain when/how the cleanup happens (at startup or with a tool, right?) Done PS5, Line 1833: actual_size > you're basically saying in the comments above that this number is unstrustw Yes, it's untrustworthy. I'll change the name. PS5, Line 1834: s = env_->GetFileSizeOnDisk(data_filename, &actual_size); : if (!s.ok()) { : *result_status = s.CloneAndPrepend(Substitute( : "Could not get on-disk file size of container $0", container->ToString())); : return; : } > use RETURN_NOT_OK_PREPEND Can't; this function is run asynchronously out of a thread pool so it doesn't return its status. PS5, Line 1840: measured_size > _this_ is the actual size, right? This is the size according to our own accounting. It's not the size according to the filesystem. PS5, Line 2054: TODO(adar): can be more efficient (less fs work and more space reclamation : // in case of misaligned blocks) via hole coalescing first, but this is easy. > file a jira? seems like we might have to revisit this if it turns out that I'm going to punt. I think the TODO() is enough for now, and my observations show that even repunching every hole in the container isn't that bad (see above). http://gerrit.cloudera.org:8080/#/c/6717/5/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: PS5, Line 116: certain > which ones? can it be any operation? The ones that use it. :) It's a little ridiculous to maintain an exhaustive list that must be kept in-sync, especially since this is a testing-only gflag. It'd also be ridiculous to say "search for all references to this flag in order to find which operations are affected." -- To view, visit http://gerrit.cloudera.org:8080/6717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I016ea401380f4c8c7b1fd907ff67cb595f377dd1 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
