Andrew Wong has posted comments on this change. Change subject: [WIP]log block manager:update container in-memory accounting after data is synced ......................................................................
Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/7374/5//COMMIT_MSG Commit Message: PS5, Line 12: data record is actually : been written to disk but fsync() does not succeed. May be helpful to go through an example scenario here 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: FLAGS_env_inject_eio = 0.1; I think you might be trying to reset the flag once it goes out of scope? If so, you'll need to create a FlagSaver in this scope. Then you won't have to reset the flag at L1006 PS5, Line 971: auto create_a_block = [&](BlockId* out, const string& test_data) -> Status { : unique_ptr<WritableBlock> block; : RETURN_NOT_OK(this->bm_->CreateBlock(this->test_block_opts_, &block)); : for (int i = 0; i < kNumAppends; i++) { : RETURN_NOT_OK(block->Append(test_data)); : } : { : FLAGS_env_inject_eio = 0.1; : RETURN_NOT_OK(block->Close()); : } : *out = block->id(); : return Status::OK(); : }; Since this is so similar to close_a_block in TestMetadataOkayDespiteFailedWrites, consider creating a member function to do this. PS5, Line 986: bool log not used? PS5, Line 985: // Reads a block given by 'id'. : auto read_a_block = [&](const BlockId& id, bool log) -> Status { : unique_ptr<ReadableBlock> block; : RETURN_NOT_OK(this->bm_->OpenBlock(id, &block)); : uint64_t size; : RETURN_NOT_OK(block->Size(&size)); : uint8_t scratch[size]; : Slice data(scratch, size); : RETURN_NOT_OK(block->Read(0, &data)); : return Status::OK(); : }; Member for this too, maybe with a num_appends field as input? Also why do we only do this for one append (vs TestMetadataOkayDespiteFailedWrites that does it multiple times)? PS5, Line 1008: f (i % 2 == 0) { : s = create_a_block(&id, long_test_data); : } else { : s = create_a_block(&id, short_test_data); Why do we need to inject data of different size here? http://gerrit.cloudera.org:8080/#/c/7374/5/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS5, Line 266: int64_t block_offset docs for this PS5, Line 357: // Updates internal bookkeeping state to reflect the creation of a block, : // marking this container as full if needed. Should only be called when a : // block is fully written, as it will round up the container data file's position. : // : // This function is thread unsafe. : void BlockCreated(const scoped_refptr<LogBlock>& block); : : void RoundUpDataFile(int64_t offset, int64_t length); seems like comments here should be updated PS5, Line 761: s Will we need to be updated when disk failure handling gets in? If so, maybe add a TODO Or explain why what non-OK errors are allowed 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
