Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 10: (38 comments) I didn't make it all the way through, but between these comments and the TSAN warnings, I think there's plenty to be done. http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 900: if (!FLAGS_cfile_close_block_on_finish) { Could you further parameterize the test so that we run with both values of this gflag? Otherwise it may bit rot for the non-default value. http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS10, Line 62: "Boolean to determine whether to close cfile blocks when " : "writing is finished. Finalize the blocks if not close."); "Whether to finalize or close cfile blocks when writing is finished." Also, rename to "cfile_close_blocks_on_finish" (since it deals with all blocks) and fix the indentation. 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: const string test_data = "test data"; kTestData for this. Above and below too. Line 1002: auto write_data = [&]() { Since these operations happen on other threads, you should use CHECK_OK instead of ASSERT_OK. Line 1007: ASSERT_OK(writer->Close()); Maybe inject a tiny (random) bit of sleeping between Finalize() and Close(), to increase the chances of overlap? You'd also get more overlap if each thread created several blocks in a loop. Just be mindful of overall test runtime; it should be a few seconds at most. PS10, Line 1010: vector<scoped_refptr<Thread> > threads; : for (int i = 0; i < 100; i++) { : scoped_refptr<Thread> t; : ASSERT_OK(Thread::Create("test", Substitute("t$0", i), : write_data, &t)); : threads.push_back(t); : } : for (const scoped_refptr<Thread>& t : threads) { : t->Join(); : } Use std::thread; they're less LOC and thus better suited for tests. Line 1032: CHECK_EQ(test_data, slice); ASSERT_EQ PS10, Line 1043: 20 "some" instead, so that if 20 changes to 30 or 40, we don't have to update the comment too. Line 1053: //ASSERT_OK(writer->Finalize()); What's this about? http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 22: #include <kudu/gutil/stl_util.h> What happened here? The older style (and location) was correct. PS10, Line 73: 1) as less I/O as : // possible Not sure what this is saying; please reword. Line 88: // The block no more may be written. "No more data may be written to the block, but it is not yet closed." PS10, Line 123: // Ensures data may not be written to the block after Finalize() is called. : // It does not guarantee durability of the block. "Signals that the block will no longer receive writes. Does not guarantee durability; Close() must still be called for that." PS10, Line 275: blocks block PS10, Line 276: 1) to gain performance optimization if possible Expand on this a bit (by describing in vagaries that the underlying block manager can optimize for a batch of operations). You can use the doc for BlockManager::CloseBlocks() as inspiration. PS10, Line 277: logic logical Line 279: public: Nit: indent by one char. Line 312: private: Nit: indent by one char. 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_ == DIRTY) { : VLOG(3) << "Finalizing block " << id(); : if (FLAGS_block_manager_flush_on_finalize) { : RETURN_NOT_OK_HANDLE_ERROR(writer_->Flush(WritableFile::FLUSH_ASYNC)); : } : } else if (state_ == CLEAN) { : VLOG(3) << "Finalizing block " << id(); : } How about inverting this: if (state_ == FINALIZED) { return Status::OK(); } VLOG(3) << "Finalizing block " << id(); if (state_ == DIRTY) { RETURN_NOT_OK_HANDLE_ERROR(writer_->Flush(WritableFile::FLUSH_ASYNC)); } state_ = FINALIZED; return Status::OK(); 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/stl_util.h" Why is this include needed? 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: ASSERT_EQ(1, bm_->all_containers_by_name_.size()); I'd also close all the blocks in 'blocks' and check all_containers_by_name_.size() again, to make sure it's still 1. Line 1277: // Test Close() a FINALIZED block. Isn't this state transition already tested by BlockManagerTest.WritableBlockStateTest? Line 1288: // Ensure the same container has not been marked as available twice. How does L1289 check this? 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 internal::LogWritableBlock; There's an existing internal::LogWritableBlock reference in the file; please change it to just LogWritableBlock. Line 228: class LogWritableBlock; Why is this forward declaration needed? Line 513: atomic<int64_t> next_sync_offset_; Please use AtomicInt rather than std::atomic, since that's what's already being used in this module. Line 984: std::lock_guard<simple_spinlock> l(lock_); We shouldn't hold spinlocks while doing IO, because IO is long running and contention on a spinning lock may consume a CPU. Use a sleeping lock for this (i.e. kudu::Mutex). Line 1194: enum SyncMode { Rename this to CloseFlags and the entries like so: EMPTY = 0, FINISH = 1 << 0, SYNC = 1 << 1, That way you can OR these flags together when calling DoClose and the meaning will be very clear. Like, DoClose(FINISH | SYNC) means "finish and sync". Line 1261: Status LogBlockContainer::SyncBlocks(const vector<WritableBlock*>& blocks) { To avoid the intermingling of LogBlockContainer and LogWritableBlock definitions, reorder in this way: 1. Forward declaration of LogBlockContainer. 2. LogWritableBlock declaration. 3. LogBlockContainer declaration and definitions. 4. LogWritableBlock definitions. You'll see we already followed this pattern (splitting the declaration and definition) for LogBlock. Ideally this reordering would happen in a separate patch before this one, so that there's a clear difference between the simple reordering and the new code. PS10, Line 1263: status Pick a name that reflects the work done by the functor. Line 1269: LogWritableBlock *lwr = down_cast<LogWritableBlock *>(block); LogWritableBlock* Actually, just change the function to accept a vector of LogWritableBlocks and rely on callers to do the down casting. PS10, Line 1344: if (container_->read_only()) { : return Status::IllegalState(Substitute("container $0 is read-only", : container_->ToString())); : } Can we make this a DCHECK? Can this actually happen? Line 1411: if (state_ == DIRTY) { Invert all of this: if (state_ == FINALIZED) { return Status::OK(); } VLOG(3) << "Finalizing block " << id(); if (state_ == DIRTY) { if (FLAGS_block_manager_flush_on_finalize) { RETURN_NOT_OK(container_->FlushData(block_offset_, block_length_)); } s = AppendMetadata(); RETURN_NOT_OK_PREPEND(s, "Unable to append block metadata"); if (FLAGS_block_manager_flush_on_finalize) { // TODO(unknown): Flush just the range we care about. s = container_->FlushMetadata(); RETURN_NOT_OK(s); } } state_ = FINALIZED; return Status::OK(); PS10, Line 1425: else if (state_ == CLEAN) { : VLOG(3) << "Finalizing block " << id(); : s = AppendMetadata(); : RETURN_NOT_OK_PREPEND(s, "Unable to append block metadata"); : } Why append metadata if the block is clean? The previous behavior was to write no block entry because the block was empty. Line 1913: for (WritableBlock *block : blocks) { WritableBlock* Line 1914: LogWritableBlock *lwr = down_cast<LogWritableBlock *>(block); LogWritableBlock* http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/tablet/deltafile.h File src/kudu/tablet/deltafile.h: PS10, Line 72: Finalize Change the tense back, so "Closes ..., finalizing ... and releasing ...". 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. Finalize the underlying writable blocks Do you mind changing this back to the tenses used before? That is, "Close <...>, finalizing <...> and releasing <...>." -- 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: 10 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
