Adar Dembo has posted comments on this change. Change subject: log block manager: corruptor test utility ......................................................................
Patch Set 3: (17 comments) http://gerrit.cloudera.org:8080/#/c/6582/3/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: PS3, Line 187: InjectNonFatalInconsistencies > docs Done http://gerrit.cloudera.org:8080/#/c/6582/3/src/kudu/fs/log_block_manager-test-util.cc File src/kudu/fs/log_block_manager-test-util.cc: Line 136: > nit: extra line Done PS3, Line 153: and metadata file > in this case the metadata file exists right? can you make that clear? Done PS3, Line 154: Empty data file and metadata file > both empty? No, the metadata file may be up to kPBContainerMinimumValidLength bytes in size. PS3, Line 184: at least > nit: redundant Done PS3, Line 213: // 0. No block offset. : // 1. No block length. : // 2. Negative block offset. : // 3. Negative block length. : // 4. Offset + length > data file size. : // 5. Two CREATEs for same block ID. : // 6. DELETE without first matching CREATE. : // 7. Unrecognized op type. > are these based on stuff we've seen, or just the ways you think this might These are all the checks done by the code in log_block_manager.cc. I'll make that explicit. PS3, Line 251: at least > nit: redundant Done PS3, Line 320: Once done > what does this refer to? It refers to the "partial record" corruption. I'll reword. PS3, Line 346: // Loop and try a different operation. > maybe add a "continue" for clarity? (might seem like this comment is referr Done http://gerrit.cloudera.org:8080/#/c/6582/3/src/kudu/fs/log_block_manager-test-util.h File src/kudu/fs/log_block_manager-test-util.h: PS3, Line 99: ContainerMode > maybe "FindContainerMode"? Done http://gerrit.cloudera.org:8080/#/c/6582/3/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: PS3, Line 92: GetOnlyContainerDataFile > docs Done PS3, Line 99: GetOnlyContainer > docs Done PS3, Line 116: AssertEmptyReport > should we add an AllOK() method to FsReport or something? I don't think it's necessary, since it'd have to manufacture a fake Status to be asserted on here. Better to just ASSERT on the arrays directly like this. PS3, Line 146: // Return what was requested. > redundant Done PS3, Line 805: several variants of "incompleteness" at random. > point to somewhere where we can find the possible incompleteness modes? Done PS3, Line 842: malformedness > same Done PS3, Line 912: // Add some partial records. : LBMCorruptor corruptor(env_, { test_dir_ }, SeedRandom()); : ASSERT_OK(corruptor.Init()); : for (int i = 0; i < kNumRecords; i++) { : ASSERT_OK(corruptor.AddPartialRecordToContainer()); : } > any way to assert that the errors were there without fixing? i.e. if you di Opening the block manager in read-only mode would check for errors without repairing them. But, what exactly would be the point of doing that, when repairing is logically the same coverage plus a little more? I don't understand the second part of your question. If the highlighted code weren't executed, there would be no inconsistencies and the report below would be empty. -- To view, visit http://gerrit.cloudera.org:8080/6582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
