Hao Hao has posted comments on this change. Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/7207/3//COMMIT_MSG Commit Message: PS3, Line 10: which happen : in a logical op > 'which happen in a logical operation' Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/bloomfile.h File src/kudu/cfile/bloomfile.h: Line 47: // Close the bloom's CFile, finalizing the underlying blocks and > Update this comment. Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/cfile_writer.h File src/kudu/cfile/cfile_writer.h: Line 109: // Close the CFile. Finalize the underlying blocks and release > Update this comment. Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 67: // Close() is an expensive operation, as it must flush both dirty block data > This comment should be updated to explain Finalize. Done Line 142: virtual size_t BytesAppended() const = 0; > Add more info about what 'finalize' means in this context, either here or i Done Line 143: > Perhaps we could simplify this interface by combining 'FlushDataAsync' and yes, there is some cases the block is flushed but not finalized, you can call FlushDataAsync() and then Close(). Or it is finalized but not flushed, in cfile writer, if FLAGS_cfile_do_on_finish is 'nothing', the block will not be flushed. Line 144: virtual State state() const = 0; > On the naming of 'FinalizeWrite' - the name is good, but I'd be in favor of Done Line 283: // nor is the order guaranteed to be deterministic. Furthermore, if > Update comment Done Line 305: STLDeleteElements(&created_blocks_); > Hmm I know we discussed on slack (or the design doc?) why CommitWrites need We discussed that in flush/compaction, superblock gets flushed in between the new rowsets get created and the old rowsets get GCed. We do not want to delete blocks that should be kept, if superblock flush fails. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 429: string metadata_path = path + LogBlockManager::kContainerMetadataFileSuffix; > Is this needed? Done Line 915: unique_ptr<WritableBlock> block; > Should this be committing the writes? Seems strange the previous version d Looking at the purpose of the tests, the writes seem not necessarily need to be committed. So I keep it the way it was. Line 1107: unique_ptr<WritableBlock> block; > Same here. Same reason above. http://gerrit.cloudera.org:8080/#/c/7207/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 24: #include <unordered_map> > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 274: > Any reason to prefer a set here to a vector? Vectors are typically lighter Was trying to only have unique block here. But given the way how this method gets called the blocks should all be unique, so will change to vector instead. Line 367: // the container data file's position, as round up could already be done via > Can't this method internally determine whether the round up has already bee Good point, but I feel it is a little bit harder than that, as we also have state as 'FLUSHING'. Also, LogBlock does not have the state info. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: Line 90: // Closes the CFiles, finalizing the underlying blocks and releasing > Update comment. 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: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
