Hao Hao has posted comments on this change. Change subject: [WIP]log block manager:mark container as 'non-writable' after IOError ......................................................................
Patch Set 7: (14 comments) http://gerrit.cloudera.org:8080/#/c/7374/7/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 166: int kNumAppends, double eio_ratio) { > warning: invalid case style for parameter 'kNumAppends' [readability-identi Done Line 888: FLAGS_never_fsync = false; > Why? Initially I turned on 'never_fsync' flag as it disabled in the test, which means fault injection for fsync() does not quite make much sense as it never turns on. But as your point out, the test does not really rely on this, so will remove it. Line 968: TYPED_TEST(BlockManagerTest, TestMetadataOkayDespiteFailedClose) { > Why does this need to be a separate test? Why can't we modify the existing I made it a separate test to explicitly test with fault injection around block Close(), while the existing test with fault injection around the life cycle of blocks. Line 969: const int kNumTries = 1; > If this is 1, why bother with a loop at all? Done PS7, Line 972: lontestdata > "longtestdata" perhaps? Also, should be kLongTestData and kShortTestData si Done Line 980: // Force some file operations to fail. > Don't we need to set env_inject_eio too? How does this work? I explicitly put the 'env_inject_eio' flag around block Close() to avoid operations already failed before reaching Close(). Updated the comment. Line 982: FLAGS_never_fsync = false; > Why? Same reason as above, Removed. Line 993: FLAGS_env_inject_eio = 0; > Isn't it already set to 0? Good catch, a mistake, will remove. Line 1000: if (s.ok()) { > Ah, I thought it was a straight-up move of create_a_block. I am good with both, will update it. Line 1008: { > What's the point of this extra scope? Done http://gerrit.cloudera.org:8080/#/c/7374/7/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS7, Line 470: but write block : // and delete block are. > Block deletion should also be forbidden (it involves writing to the metadat Typo.. it should be read operation... If we forbid delete operation that means the block can never be deleted? PS7, Line 764: as the in-memory : // accounting may be in an inconsistent state > That's not really the reason for forbidding future writes to the container Right.. will update it with more precise description. Line 766: if (PREDICT_FALSE(s.IsIOError())) { > Why single out IOError? Shouldn't any non-OK status mark the container as u Make sense, will update. http://gerrit.cloudera.org:8080/#/c/7374/7/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 329: // Inject error after fsync() is done to simulate cases where > I don't think this is worth simulating, because short of a kernel panic or You are right, this does not quite relate to the test, as the test does not rely on power failure to a machine or similar. -- To view, visit http://gerrit.cloudera.org:8080/7374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15062b9d82c9cb563fb6bb2af7ec89da5f71e28f Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
