Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] limit number of rows when dumping a rowset ...................................................................... Patch Set 3: (13 comments) 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 number of rows when dumping a rowse > limit number of rows when dumping a rowset Done http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@10 PS2, Line 10: if --dump_all_columns is enab > if --dump_all_columns is not enabled Done http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@8 PS2, Line 8: : The --nrows flag for 'local_replica dump rowset' doesn't : work if --dump_all_columns is enab > If FLAGS_dump_all_columns is not enabled, then Thansk! 1. You're right, the description is incorrect. Fixed. 2. See the reply in https://gerrit.cloudera.org/c/19369/2/src/kudu/tools/tool_action_local_replica.cc#b1048 http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@12 PS2, Line 12: This patch fixes the issue, so now the 'local_replica dump : rowset' CLI to > This patch fixes the issue, so now the 'local_replica dump rowset' CLI tool Done 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: #include <cstdint> > nit: remove this empty extra line? Oops, all of them are C++ standard libs. Done. http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/compaction.h@261 PS2, Line 261: If 'rows_left' is nullptr, there is no limit on the number of rows to dump. : // If the content > This consumes no more rows from the compaction input than specified by the Done 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 Done 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: Status DebugDump(int64_t* rows_left, std::vector<std::string> *lines) override; : > nit: update the comment to document the newly added parameter I removed all the comments of sub-classes of this function and updated the comments of super class (class RowSet). 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: The Tidy Bot report like: > warning: default arguments on virtual or override methods are prohibited > [google-default-arguments] http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tablet/rowset.h@180 PS2, Line 180: ptr, there is no l > Would be great to document this parameter. Does a negative value for *nrow Done 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@a1048 PS2, Line 1048: > It seems after the '}' add a line: No, in the prior, there is no limit on RowSet::DebugDump, so even if *row_left == 1, the rowset will dump all rows in the rowset, which number is larger than 1. http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tools/tool_action_local_replica.cc@987 PS2, Line 987: > printed Done http://gerrit.cloudera.org:8080/#/c/19369/2/src/kudu/tools/tool_action_local_replica.cc@987 PS2, Line 987: > printed Done -- 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: 3 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: Sun, 08 Jan 2023 17:16:33 +0000 Gerrit-HasComments: Yes
