Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 21: (30 comments) http://gerrit.cloudera.org:8080/#/c/7207/19//COMMIT_MSG Commit Message: PS19, Line 22: --nu > You still need 'localhost' though; that's not an optional parameter I think Done http://gerrit.cloudera.org:8080/#/c/7207/20/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: Line 282: AtomicInt<int64_t> total_bytes_written_; > Should be <const uint8_t*> Done PS20, Line 297: vector<unique_ptr<WritableBlock>> all_dirty_blocks; : for (int i = 0; i < FLAGS_block_group_number; i++) { > You changed the pointer/ref style here. Done Line 310: > Update. Done Line 311: dirty_blocks.emplace_back(std::move(block)); > const auto& Done Line 318: // random, and write a smaller chunk of data to it. > auto& Done Line 326: // Write a small chunk of data. > const auto& Done http://gerrit.cloudera.org:8080/#/c/7207/20/src/kudu/fs/block_manager.cc File src/kudu/fs/block_manager.cc: PS20, Line 44: // throughput but may improve write latency. : DEFINE_string(block_manager_flush_control, "finalize", : "Controls when to flush a block. Valid values are 'finalize', " : "'close', or 'never'. If 'finalize', blocks will be flushed " : "when writing is finished. If 'close', blocks will be > Rewrite: Done http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1410: > Okay, but why bother maintaining FlushDataAsync() as a separate method? It Makes sense. Updated. But still kept the per-impl methods in LogWritableBlock/FileWritableBlock since calling Flush() from LogBlockManager/FileBlockManager will result in more code if not. http://gerrit.cloudera.org:8080/#/c/7207/20/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS20, Line 260: onst OVERRIDE; > finalizing it if it has not yet been finalized. Done PS20, Line 260: > "Also updates" Done PS20, Line 336: c > , Done PS20, Line 337: > the Done PS20, Line 434: // Malformed records and other container inconsistencies are written to : // 'report'. Healthy blocks are written either to 'live_blocks' or : // 'dead_blocks'. Live records are written to 'live_block_recor > Nit: would prefer to keep BlockCreated and BlockDeleted next to each other Done PS20, Line 543: // Offset up to which we have preallocated bytes. : int64_t preallocated_offset_ = 0; > Since all counters are now atomic, these comments are no longer interesting Done PS20, Line 878: at as a d > "data" (to be consistent with "Syncing metadata file") Done PS20, Line 882: > Replace with "is already made durable". Done PS20, Line 883: > const auto* Done PS20, Line 890: if (mode == SYNC) > I don't think this one needs to be conditioned on SYNC. Done Line 891: > Would be nice if we could DCHECK that if blocks.size() > 1, all of the bloc Done PS20, Line 1040: preallocated_offset_ = off + len; : } : > This needs to be updated. Done Line 1068: DCHECK_GE(block_offset, 0); > Don't need the if statement anymore. Done PS20, Line 1071: // means accounting for the new block should be as simple as adding its : // aligned length to 'next > Let's defer this to BlockCreated(). Then RoundUpContainer can be renamed to Done PS20, Line 1206: etrics > Nit: { this } Done Line 1216: } > Nit: { this } Done PS20, Line 1289: } : : Status LogWritableBl > Rewrite: "We do not mark the container as read-only if FlushDataAsync() fai Done Line 1317: } > 'lbm' is only used once; don't bother storing it in a local variable. Done PS20, Line 1319: state() const { > Just use block_length_ here; it's more clear. Done PS20, Line 1319: te L > and block_id_ here. Done PS20, Line 1751: ol == "close") { > Nit: <LogWritableBlock*> 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: 21 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
