Todd Lipcon has posted comments on this change. Change subject: KUDU-1793: only update lbm container bookkeeping on success ......................................................................
Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/5399/3/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: PS3, Line 1160: kNumAppendTries nit: sounds like multiple attempts/retries at the same operation. Maybe kNumAppends is better PS3, Line 1168: kTestdata nit: kTestData PS3, Line 1179: sizeof nit: arraysize is better than sizeof PS3, Line 1179: 0 erm? http://gerrit.cloudera.org:8080/#/c/5399/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS3, Line 206: next_append_bytes I find this parameter name a little confusing. maybe next_append_size or next_append_length? Line 672: RETURN_NOT_OK(data_dir_->RefreshIsFull(DataDir::RefreshMode::ALWAYS)); not a new issue, but question here: if preallocation is not enabled, does this mean that every write also causes a statvfs call? is that a big deal? http://gerrit.cloudera.org:8080/#/c/5399/3/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: PS3, Line 110: file_allocation_injected_failure the other injection flags follow a pattern more like 'env_inject_error_on_fallocate' or somesuch PS3, Line 371: RuntimeError I think an IOError would be more consistent with the status type that is returnable below Line 576: MAYBE_RETURN_FAILURE(FLAGS_file_allocation_injected_failure, the name here is not quite matching up. Perhaps the flag should be clearer that it's injecting on _any_ data write operation? -- To view, visit http://gerrit.cloudera.org:8080/5399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I49bc98c9f8b7dce0333f88cec85757fe122acfa4 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
