Hao Hao has posted comments on this change. Change subject: [WIP]log block manager:mark container as 'non-writable' after IOError ......................................................................
Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/7374/5/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: PS5, Line 978: S_log_container_preallocate > I think you might be trying to reset the flag once it goes out of scope? If Done PS5, Line 971: const int kNumAppends = 1000; : const string long_test_data = "lontestdata"; : const string short_test_data = "data"; : : // Since we're appending small data, reconfigure these to ensure quite a : // few containers and a good amount of preallocating. : FLAGS_log_container_max_size = 256 * 1024; : FLAGS_log_container_preallocate_bytes = 8 * 1024; : : // Force some file operations to fail. : FLAGS_suicide_on_eio = false; : FLAGS_never_fsync = false; : > Since this is so similar to close_a_block in TestMetadataOkayDespiteFailedW Done PS5, Line 986: > not used? Done PS5, Line 985: // 1. Try to create kNumTries new blocks. : // 2. Read and test every block. : // 3. Restart the block manager, forcing the on-disk metadata to be reloaded. : BlockIdSet ids; : for (int attempt = 0; attempt < kNumTries; attempt++) { : int num_created = 0; : for (int i = 0; i < kNumBlockTries; i++) { : BlockId id; : FLAGS_env_inject_eio = 0; : Status s; : > Member for this too, maybe with a num_appends field as input? Because TestMetadataOkayDespiteFailedWrites only inject one type of data, so it is possible to verify the content while in this case it is not easy to do so. So in this test it only verifies if the Read() is successful. PS5, Line 1008: : for (const auto& id : ids) { : ASSERT_OK(this->ReadBlock(id)); : } > Why do we need to inject data of different size here? The idea is without the fix the test can result in malformed record. Assume, we have block A written first but failed and follow with block B but succeed in a container. Block A has larger size of data than Block B and the metadata of block A is 'persistent'. When reopen BM, it can find bad record by checking that offset + length > data_file_size. Inject the same size of data theoretically may also have the same error, however, after several experience, different size have better chance to detect this issue. I will add a comment here. -- 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
