Adar Dembo 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 kNu Done PS3, Line 1168: kTestdata > nit: kTestData Done PS3, Line 1179: sizeof > nit: arraysize is better than sizeof Doesn't seem to work: /home/adar/Source/kudu/src/kudu/fs/block_manager-test.cc:1191:92: error: no matching function for call to ‘ArraySizeHelper(uint8_t [(<anonymous> + 1)])’ In file included from /home/adar/Source/kudu/src/kudu/fs/block_id.h:26:0, from /home/adar/Source/kudu/src/kudu/fs/file_block_manager.h:26, from /home/adar/Source/kudu/src/kudu/fs/block_manager-test.cc:24: /home/adar/Source/kudu/src/kudu/gutil/macros.h:142:8: note: candidate: template<class T, long unsigned int N> char (& ArraySizeHelper(T (&)[N]))[N] char (&ArraySizeHelper(T (&array)[N]))[N]; ^ /home/adar/Source/kudu/src/kudu/gutil/macros.h:142:8: note: template argument deduction/substitution failed: /home/adar/Source/kudu/src/kudu/fs/block_manager-test.cc:1191:92: note: variable-sized array type ‘long int’ is not a valid template argument In file included from /home/adar/Source/kudu/src/kudu/fs/block_id.h:26:0, from /home/adar/Source/kudu/src/kudu/fs/file_block_manager.h:26, from /home/adar/Source/kudu/src/kudu/fs/block_manager-test.cc:24: /home/adar/Source/kudu/src/kudu/gutil/macros.h:149:8: note: candidate: template<class T, long unsigned int N> char (& ArraySizeHelper(const T (&)[N]))[N] char (&ArraySizeHelper(const T (&array)[N]))[N]; ^ /home/adar/Source/kudu/src/kudu/gutil/macros.h:149:8: note: template argument deduction/substitution failed: /home/adar/Source/kudu/src/kudu/fs/block_manager-test.cc:1191:92: note: variable-sized array type ‘long int’ is not a valid template argument PS3, Line 1179: 0 > erm? Ugh, that should have been 'i', not 0. 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 ne Will use 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 t Yes, if you've disabled preallocation, you're getting a statvfs() with every write. As for whether or not it's a big deal, I don't know. I'd expect that information to be cached, so it'd be the overhead of an extra syscall at worst. I haven't fretted about it because disabling preallocation just seems like a bad idea in general. 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_f Will rename to env_inject_io_error_on_write_or_preallocate. PS3, Line 371: RuntimeError > I think an IOError would be more consistent with the status type that is re Done Line 576: MAYBE_RETURN_FAILURE(FLAGS_file_allocation_injected_failure, > the name here is not quite matching up. Perhaps the flag should be clearer I think the new name (see earlier comment) makes it clear. -- 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
