Zoltan Martonka has posted comments on this change. ( http://gerrit.cloudera.org:8080/22406 )
Change subject: WIP [compaction] Helper methods and skeleton code for compaction batching ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/delta_compaction.h File src/kudu/tablet/delta_compaction.h: http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/delta_compaction.h@86 PS2, Line 86: // Returns true if current iteration of compaction is processing limited number of : // delta blocks in a batch. : bool chunked_compaction() { : return chunked_compaction_; : } : : // Return pair of start and end row ids in current batch of delta blocks. : std::pair<rowid_t, rowid_t> rowspan_in_batch() { : return rowspan_in_batch_; : } : : // Return last block pointer from previous batch of delta blocks. : cfile::BlockPointer last_blk_ptr() { : return last_blk_ptr_; : } [f] all 3 functions should be const functions [f] const std::pair<rowid_t, rowid_t>& return value? http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/delta_store.h@318 PS2, Line 318: al bool chunked_compaction() = 0; : : // Return pair of start and end row ids in current batch of delta blocks. : virtual std::pair<rowid_t, rowid_t> rowspan_in_batch() = 0; : : // Return last block pointer from previous batch of delta blocks. : virtual cfil [f] all 3 function can be const ? [f] const Blocpointer& return value (unless there is an use case, where it is not just a simple getter) http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/delta_store.h@490 PS2, Line 490: / Returns true if current iteration of compaction is processing limited number of : // delta blocks in a batch. : bool chunked_compaction() override; : : // Return pair of start and end row ids in current batch of delta blocks. : std::pair<rowid_t, rowid_t> rowspan_in_batch() override; : : // Return last block pointer from previous batch of delta blocks. : cfile::BlockPointer last_blk_ptr() override; [f] I think it better not to add the exactly same comment to the override. Only derived class specific comments should come here. http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/deltamemstore.cc File src/kudu/tablet/deltamemstore.cc: http://gerrit.cloudera.org:8080/#/c/22406/2/src/kudu/tablet/deltamemstore.cc@23 PS2, Line 23: #include <utility> Already included in header? -- To view, visit http://gerrit.cloudera.org:8080/22406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I111d613ec717534df9ba09e5660420b5d6c9fed0 Gerrit-Change-Number: 22406 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Wed, 29 Jan 2025 16:18:51 +0000 Gerrit-HasComments: Yes
