David Ribeiro Alves has posted comments on this change. Change subject: fs: generate report during Open ......................................................................
Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: PS4, Line 69: // unnecessarily. > Generally I wrap at 80 characters, and this extends to 81 if unwrapped. But I don't think that it makes a lot of sense for people to have different wrapping policies. Not saying that 80 chars is wrong or right, just that we should all do it or none of us should do it. Given that code style mandates wrapping at 120 I usually point as nits forced wrapping well below that where there's a single word in the last line. particularly when wrapping occurs in places where you can find surrounding code that doesn't, like this one. In general, if devs need to have their displays adjusted to 120 cols anyway, vertical space is more precious than horizontal space, IMO http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS4, Line 324: truly catastrophic > This isn't for fatal and irreparable inconsistencies though; this is for "w can you spell that out then? it's unlikely that the next person using this will know what you mean by "truly catastrophic" PS4, Line 584: wildly > Non-existent or negative values (what we're testing for in the subsequent c can you spell that out then, please? (that's kind of what I meant from my comment) PS4, Line 1726: Truncation is performed even if the container's logical file size : // (available by proxy via preallocated_offset_) and total_bytes_written_ : // agree, because XFS's speculative preallocation feature may artificially : // enlarge the file without updating its file size. > Why? It's just one sentence and you can't read it without seeing "XFS specu Yeah, but if it starts with "For XFS filesystems" or somesuch, then you know you don't need to read it at all if what you're concerned about is another filesystem PS4, Line 1753: // TODO(adar): track as an inconsistency? > I hesitated to track it as an inconsistency for it for a couple reasons: sounds good -- To view, visit http://gerrit.cloudera.org:8080/6581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
