Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 12: (38 comments) http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 900: } > Could you further parameterize the test so that we run with both values of Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS10, Line 62: "Whether to finalize or close cfile blocks when writing " : "is finished. "); > "Whether to finalize or close cfile blocks when writing is finished." Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 998: TYPED_TEST(BlockManagerTest, ConcurrentCloseFinalizedWritableBlockTest) { > kTestData for this. Above and below too. Done Line 1002: // written, and then Close() it. > Since these operations happen on other threads, you should use CHECK_OK ins Done Line 1007: CHECK_OK(writer->Append(kTestData)); > Maybe inject a tiny (random) bit of sleeping between Finalize() and Close() Done PS10, Line 1010: CHECK_OK(writer->Close()); : } : }; : : vector<std::thread> threads; : for (int i = 0; i < 100; i++) { : threads.emplace_back(write_data); : } : for (auto& t : threads) { : > Use std::thread; they're less LOC and thus better suited for tests. Done Line 1032: ASSERT_OK(block->Read(0, &slice)); > ASSERT_EQ Done PS10, Line 1043: a > "some" instead, so that if 20 changes to 30 or 40, we don't have to update Done Line 1053: ASSERT_OK(writer->Append(kTestData)); > What's this about? My bad, comment it out for testing, but forget to uncomment back. http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 22: #include <memory> > What happened here? The older style (and location) was correct. Done PS10, Line 73: 1) flushing of dirty : // blocks a > Not sure what this is saying; please reword. Done Line 88: // No more data may be written to the block, but it is not yet closed. > "No more data may be written to the block, but it is not yet closed." Done PS10, Line 123: // Signals that the block will no longer receive writes. Does not guarantee : // durability; Close() must still be called for t > "Signals that the block will no longer receive writes. Does not guarantee d Done PS10, Line 275: block > block Done PS10, Line 276: 1) the underlying block manager can optimize > Expand on this a bit (by describing in vagaries that the underlying block m Done PS10, Line 277: possi > logical Done Line 279: class BlockTransaction { > Nit: indent by one char. Done Line 312: } > Nit: indent by one char. Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: PS10, Line 335: : if (state_ == FINALIZED) { : return Status::OK(); : } : VLOG(3) << "Finalizing block " << id(); : if (state_ == DIRTY) { : if (FLAGS_block_manager_flush_on_finalize) { : > How about inverting this: Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: Line 38: #include "kudu/gutil/stringprintf.h" > Why is this include needed? Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 1274: } > I'd also close all the blocks in 'blocks' and check all_containers_by_name_ Done Line 1277: for (const auto& block : blocks) { > Isn't this state transition already tested by BlockManagerTest.WritableBloc I combined this test with the test case above. Let me know if you still think it is redundant. Thanks! Line 1288: } // namespace fs > How does L1289 check this? I was trying to check in the available_containers_by_data_dir_ map, there is only one container for the corresponding datadir(should be the first element of the map). Maybe there is a more elegant way to do so? http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 129: using pb_util::WritablePBContainerFile; > There's an existing internal::LogWritableBlock reference in the file; pleas Done Line 513: // The metrics. Not owned by the log container; it has the same lifespan > Please use AtomicInt rather than std::atomic, since that's what's already b Done Line 984: // is we sync the data/metadata more often than needed. > We shouldn't hold spinlocks while doing IO, because IO is long running and Done Line 1194: // persisted. > Rename this to CloseFlags and the entries like so: Done Line 1261: > To avoid the intermingling of LogBlockContainer and LogWritableBlock defini Make sense, will do it in a separate patch. PS10, Line 1263: > Pick a name that reflects the work done by the functor. Done Line 1269: RETURN_NOT_OK(Sync()); > LogWritableBlock* Done PS10, Line 1344: DCHECK(state_ == CLEAN || state_ == DIRTY) : << "Invalid state: " << state_; : : i > Can we make this a DCHECK? Can this actually happen? This now could happen if Finalize() is successful but Close() is not. That means the container is read-only but is also marked as available. And another block can actually try to write to it. Line 1411: lbm->metrics()->full_containers->Increment(); > Invert all of this: Done PS10, Line 1425: s = AppendMetadata(); : RETURN_NOT_OK_PREPEND(s, "Unable to append block metadata"); : : if (FLAGS_block_manager_flush_on_finalize) { : > Why append metadata if the block is clean? The previous behavior was to wri I feel there is some issue with the old behavior, as if FlushDataAsync()/Finalize() is called on a clean block, the state of the block will be FLUSHING/FINALIZED instead of CLEAN. And when Close() is called, the metadata will no longer be appended. Line 1913: RETURN_NOT_OK(block->Finalize()); > WritableBlock* Done Line 1914: } > LogWritableBlock* Done http://gerrit.cloudera.org:8080/#/c/7207/11/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 132: using std::string; > warning: using decl 'atomic' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/tablet/deltafile.h File src/kudu/tablet/deltafile.h: PS10, Line 72: finalizi > Change the tense back, so "Closes ..., finalizing ... and releasing ...". Done http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/tablet/multi_column_writer.h File src/kudu/tablet/multi_column_writer.h: Line 68: // Close the in-progress CFiles, finalizing the underlying writable > Do you mind changing this back to the tenses used before? That is, "Close < Done -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a88884bd Gerrit-PatchSet: 12 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
