Adar Dembo has posted comments on this change. Change subject: [WIP]log block manager:mark container as 'non-writable' after IOError ......................................................................
Patch Set 7: (22 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 888: FLAGS_never_fsync = false; Why? Line 968: TYPED_TEST(BlockManagerTest, TestMetadataOkayDespiteFailedClose) { Why does this need to be a separate test? Why can't we modify the existing test to make it appropriate for this? They're almost exactly the same... Line 969: const int kNumTries = 1; If this is 1, why bother with a loop at all? PS7, Line 972: lontestdata "longtestdata" perhaps? Also, should be kLongTestData and kShortTestData since these are constants. Line 980: // Force some file operations to fail. Don't we need to set env_inject_eio too? How does this work? Line 982: FLAGS_never_fsync = false; Why? Line 993: FLAGS_env_inject_eio = 0; Isn't it already set to 0? Line 1000: if (s.ok()) { Why would block creation fail if no errors are being injected? Line 1008: { What's the point of this extra scope? 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 metadata file). What do you mean by "write block"; how is that different from "write operation"? Line 472: AtomicBool non_writable_; Maybe "no_longer_writable_"? Or "read_only_"? 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 though; it's because on-disk state may contain partial data/metadata and we can't safely overwrite it or append to it. Line 766: if (PREDICT_FALSE(s.IsIOError())) { Why single out IOError? Shouldn't any non-OK status mark the container as unusable? Line 767: non_writable_.CompareAndSet(false, true); Could just Store(true) here. PS7, Line 778: // A failure when syncing the container means the container (and its new : // blocks) may be missing the next time the on-disk state is reloaded. : // : // As such, it's not correct to add the block to in-memory state unless : // synchronization succeeds. However, on the other hand, it is not : // certain that the syncing failure indicates the new blocks' metadata : // are not actually on disk. Thus, it is better to mark the container : // non-writable to avoid further in-memory state inconsistency. See : // 'cleanup' above. This entire comment should be rewritten in light of the new behavior. If SyncContainer fails, it's a fatal as far as this container is concerned. Line 787: RETURN_NOT_OK(block_manager()->SyncContainer(*this)); A failure here won't actually lead to non_writable_ being set because we aren't updating 's' before returning. You'll need to change this. Line 827: CHECK(!non_writable_.Load()); Why are these new assertions CHECK and not DCHECK? Line 858: // Note: We don't check for sufficient disk space for metadata writes in We shouldn't allow metadata appends either. Line 968: void LogBlockContainer::BlockDeleted(const scoped_refptr<LogBlock>& block) { Shouldn't allow this either. Line 1198: RETURN_NOT_OK_PREPEND(AppendMetadata(), "Unable to append block metadata"); We should mark the container as unusable if this fails too, or if FlushMetadata fails. Alternatively, we can stop appending/flushing metadata here, and always defer it to DoClose(). I don't think there'd be a performance difference either way. Line 1614: scoped_refptr<LogBlock> lb(RemoveLogBlock(block_id)); This will have to change to take non-writable containers into consideration: an attempt to delete a block in a non-writable container should fail without changing any in-memory state. 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 power failure, a failure before fsync() is the same as a failure after. There are some failures that are worth simulating after operations (like pwritev() or fallocate()), but this isn't one of them. Besides, how does this relate to your test, or to this fix? -- 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
