Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Limit to count of rows to dump accurately ...................................................................... Patch Set 2: (11 comments) Thank you for the fix! Just nits, overall looks good. http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@7 PS2, Line 7: Limit to count of rows to dump accurately limit number of rows when dumping a rowset http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@10 PS2, Line 10: if not set --dump_all_columns if --dump_all_columns is not enabled http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@12 PS2, Line 12: This patch fix the issue and early break when row count : reach --nrows. This patch fixes the issue, so now the 'local_replica dump rowset' CLI tool dumps no more than the specified number of rows. http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/compaction.h@20 PS2, Line 20: nit: remove this empty extra line? http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/compaction.h@261 PS2, Line 261: This consumes partial or all of the input in the compaction input, depends : // on 'rows_left'. This consumes no more rows from the compaction input than specified by the 'rows_left' parameter. http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/compaction.h@263 PS2, Line 263: rows_left Is negative value has the 'no limit' semantics in here? It would be great to document this in the comment above. http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/memrowset.h@381 PS2, Line 381: // Dump the contents of the memrowset to the given vector. : // If 'lines' is NULL, dumps to LOG(INFO). nit: update the comment to document the newly added parameter http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/rowset.h@a180 PS2, Line 180: Just curious why not to keep this parameter by default? http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/rowset.h@180 PS2, Line 180: int64_t* rows_left Would be great to document this parameter. Does a negative value for *nrows_left has the 'no limit' semantics here? http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tools/tool_action_local_replica.cc@987 PS2, Line 987: print printed http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tools/tool_action_local_replica.cc@987 PS2, Line 987: print printed -- To view, visit http://gerrit.cloudera.org:8080/19369 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279 Gerrit-Change-Number: 19369 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 04 Jan 2023 06:34:29 +0000 Gerrit-HasComments: Yes
