You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2022/12/17 10:44:51 UTC
[kudu-CR] [tools] Limit to count of rows to dump accurately
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19369
Change subject: [tools] Limit to count of rows to dump accurately
......................................................................
[tools] Limit to count of rows to dump accurately
The --nrows flag for 'local_replica dump rowset' doesn't
work if not set --dump_all_columns.
This patch fix the issue and early break when row count
reach --nrows.
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
14 files changed, 77 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/1
--
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: newchange
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 8: Code-Review+2
--
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: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 03:00:40 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Limit to count of rows to dump accurately
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du 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:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG
Commit Message:
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 not set --dump_all_columns
If FLAGS_dump_all_columns is not enabled, then
https://gerrit.cloudera.org/c/19369/2/src/kudu/tools/tool_action_local_replica.cc#b1040~ b1044 would dump the lines, right? If yes, the describe words should fix.
If FLAGS_dump_all_columns is enabled, then
https://gerrit.cloudera.org/c/19369/2/src/kudu/tools/tool_action_local_replica.cc#b1003, would work.
IMO, reason of the command not work is because rows_left < 0 means unlimited, while
https://gerrit.cloudera.org/c/19369/2/src/kudu/tools/tool_action_local_replica.cc#b1046~ b1048, L1047 will make
*row_left < 0.
So a simple method may be:
`
if (*rows_left >= 0) {
*rows_left -= limit;
}
*row_left = std::max<int64_t>(*rows_left, 0);
`
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:
`
*row_left = std::max<int64_t>(*rows_left, 0);
`
May fix the bug. If so, and if other changes has no benefit, they can be keep it as it is .
--
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 <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 09:56:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.h@180
PS6, Line 180: // If 'rows_left' is nullptr, there is no limit on the number of rows to dump.
: // If the content of 'rows_left' equal to or less than 0, no rows will be dumped.
> It seems this part is not relevant for this function anymore.
Done
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.cc@53
PS6, Line 53:
> style nit: the indent here should be 2 spaces, not 4
Done
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1037
PS6, Line 1037: No matter if there are any rows to be printed, the metadata will be printed.
> When the --dump_metadata flag is set to 'true', the metadata is always prin
Done
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1091
PS6, Line 1091: It's not needed to read the next block when reach rows limit
> How about:
Done
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1097
PS6, Line 1097: larger
> nit: greater
Done
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1097
PS6, Line 1097: will always been
: // printed in pairs.
> ... are always printed in pairs
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: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 08:46:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] limit number of rows when dumping a rowset
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
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:
> No, in the prior, there is no limit on RowSet::DebugDump, so even if *row_l
I meant it's able to early break instead of collect all rows and limit the output at last when printing.
--
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 <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 09 Jan 2023 00:35:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 8: Code-Review+1
--
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: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 01:44:23 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
[tools] Return immediately after the limit number of rows have been dumped
The --nrows flag for 'local_replica dump rowset'
just limits the last output row count, but doesn't
take effect on the row count dumped from rowsets
internally of the tool, due to which there maybe
too much useless work.
This patch improves it by returning immediately
after --nrows of rows have been dumped..
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Reviewed-on: http://gerrit.cloudera.org:8080/19369
Tested-by: Kudu Jenkins
Reviewed-by: Wang Xixu <14...@qq.com>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
12 files changed, 111 insertions(+), 42 deletions(-)
Approvals:
Kudu Jenkins: Verified
Wang Xixu: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 6: Code-Review+1
--
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: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 06:07:22 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Ashwani Raina, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#8).
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
[tools] Return immediately after the limit number of rows have been dumped
The --nrows flag for 'local_replica dump rowset'
just limits the last output row count, but doesn't
take effect on the row count dumped from rowsets
internally of the tool, due to which there maybe
too much useless work.
This patch improves it by returning immediately
after --nrows of rows have been dumped..
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
12 files changed, 111 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/8
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Ashwani Raina, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#4).
Change subject: [tools] return immediately after the limit number of rows have been dumped
......................................................................
[tools] return immediately after the limit number of rows have been dumped
The --nrows flag for 'local_replica dump rowset'
just limits the last output row count, but doesn't
take effect on the row count dumped from rowsets
internally of the tool, there maybe too much useless
work.
This patch improve it to return immediately after
--nrows of rows have been dumped..
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
14 files changed, 90 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/4
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] Limit to count of rows to dump accurately
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#2).
Change subject: [tools] Limit to count of rows to dump accurately
......................................................................
[tools] Limit to count of rows to dump accurately
The --nrows flag for 'local_replica dump rowset' doesn't
work if not set --dump_all_columns.
This patch fix the issue and early break when row count
reach --nrows.
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
14 files changed, 77 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/2
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Ashwani Raina, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#5).
Change subject: [tools] return immediately after the limit number of rows have been dumped
......................................................................
[tools] return immediately after the limit number of rows have been dumped
The --nrows flag for 'local_replica dump rowset'
just limits the last output row count, but doesn't
take effect on the row count dumped from rowsets
internally of the tool, there maybe too much useless
work.
This patch improve it by returning immediately after
--nrows of rows have been dumped..
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
14 files changed, 90 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/5
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19369/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/5//COMMIT_MSG@7
PS5, Line 7: Return
> Use upper case?
Done
http://gerrit.cloudera.org:8080/#/c/19369/5/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/19369/5/src/kudu/tablet/memrowset.cc@151
PS5, Line 151: Status MemRowSet::DebugDumpImpl(int64_t* rows_left, vector<string>* lines) {
> How about defining 2 functions:
Tha sounds a good idea, 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: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 13 Jan 2023 08:21:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] limit number of rows when dumping a rowset
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina 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:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19369/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/3//COMMIT_MSG@9
PS3, Line 9: The --nrows flag for 'local_replica dump rowset' doesn't
: work if --dump_all_columns is enabled.
I am not able to understand from patch why dumping of rowset doesn't work with "dump_all_columns" flag.
It would be great if you could capture that in here.
http://gerrit.cloudera.org:8080/#/c/19369/3/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/19369/3/src/kudu/tools/tool_action_local_replica.cc@a1090
PS3, Line 1090:
Considering default values for rows_left as -1 and dump_all_columns set to true, what was stopping DebugDump above(at line no. 1053) from dumping all rows?
--
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 <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 11 Jan 2023 18:30:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] limit number of rows when dumping a rowset
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang 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: Code-Review+1
--
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 <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 11 Jan 2023 07:19:27 +0000
Gerrit-HasComments: No
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Ashwani Raina (Code Review)" <ge...@cloudera.org>.
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 7: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19369/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/6//COMMIT_MSG@12
PS6, Line 12: there
nit: How about - "due to which there" ?
http://gerrit.cloudera.org:8080/#/c/19369/6//COMMIT_MSG@15
PS6, Line 15: improve
nit: improves
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/compaction.h@262
PS6, Line 262: equal
nit: "is equal"
--
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: 7
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 08:59:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Ashwani Raina, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#6).
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
[tools] Return immediately after the limit number of rows have been dumped
The --nrows flag for 'local_replica dump rowset'
just limits the last output row count, but doesn't
take effect on the row count dumped from rowsets
internally of the tool, there maybe too much useless
work.
This patch improve it by returning immediately after
--nrows of rows have been dumped..
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
12 files changed, 107 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/6
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Ashwani Raina, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#7).
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
[tools] Return immediately after the limit number of rows have been dumped
The --nrows flag for 'local_replica dump rowset'
just limits the last output row count, but doesn't
take effect on the row count dumped from rowsets
internally of the tool, there maybe too much useless
work.
This patch improve it by returning immediately after
--nrows of rows have been dumped..
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
12 files changed, 111 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/7
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] return immediately after the limit number of rows have been dumped
Posted by "Wang Xixu (Code Review)" <ge...@cloudera.org>.
Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19369/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/5//COMMIT_MSG@7
PS5, Line 7: return
Use upper case?
http://gerrit.cloudera.org:8080/#/c/19369/5/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/19369/5/src/kudu/tablet/memrowset.cc@151
PS5, Line 151: Status MemRowSet::DebugDump(int64_t* rows_left, vector<string> *lines) {
How about defining 2 functions:
DebugDumpImpl(int64_t* rows_left, vector<string> *lines)
DebugDump(vector<string> *lines)
DebugDump() will call DebugDumpImpl(nullptr, &lines) to implement the origin logic.
And DebugDumpImpl() can be used by dumping rowset.
The benefits is:
DebugDump() no need to change the interface, functions using it no need to be modified.
--
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: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 13 Jan 2023 03:02:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19369/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/3//COMMIT_MSG@9
PS3, Line 9: The --nrows flag for 'local_replica dump rowset'
: just limits the last output row count,
> I am not able to understand from patch why dumping of rowset doesn't work w
Done
http://gerrit.cloudera.org:8080/#/c/19369/3/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/19369/3/src/kudu/tools/tool_action_local_replica.cc@a1090
PS3, Line 1090:
> Considering default values for rows_left as -1 and dump_all_columns set to
I got your point, the commit message has been improved.
--
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: 4
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Thu, 12 Jan 2023 12:25:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 6: Code-Review+1
(6 comments)
A few nits left to fix, otherwise looks good to me.
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.h@180
PS6, Line 180: // If 'rows_left' is nullptr, there is no limit on the number of rows to dump.
: // If the content of 'rows_left' equal to or less than 0, no rows will be dumped.
It seems this part is not relevant for this function anymore.
Probably, the whole doc for this function now needs to be moved to line 313? And this comment updated, removing this irrelevant part?
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/rowset.cc@53
PS6, Line 53:
style nit: the indent here should be 2 spaces, not 4
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1037
PS6, Line 1037: No matter if there are any rows to be printed, the metadata will be printed.
When the --dump_metadata flag is set to 'true', the metadata is always printed regardless of the number of rows to print.
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1091
PS6, Line 1091: It's not needed to read the next block when reach rows limit
How about:
There is no need to read the next block when the row limit has been reached.
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1097
PS6, Line 1097: larger
nit: greater
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tools/tool_action_local_replica.cc@1097
PS6, Line 1097: will always been
: // printed in pairs.
... are always printed in pairs
--
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: 6
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 07:04:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] Limit to count of rows to dump accurately
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 04 Jan 2023 06:34:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] limit number of rows when dumping a rowset
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
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 <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sun, 08 Jan 2023 17:16:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] limit number of rows when dumping a rowset
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yuqi Du, Yifan Zhang, Kudu Jenkins, KeDeng, Wang Xixu,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19369
to look at the new patch set (#3).
Change subject: [tools] limit number of rows when dumping a rowset
......................................................................
[tools] limit number of rows when dumping a rowset
The --nrows flag for 'local_replica dump rowset' doesn't
work if --dump_all_columns is enabled.
This patch fixes the issue, so now the 'local_replica dump
rowset' CLI tool dumps no more than the specified number of
rows.
Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
14 files changed, 81 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/19369/3
--
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: newpatchset
Gerrit-Change-Id: Ia758ba910fccbbc06ac6c59a795574fb86d4e279
Gerrit-Change-Number: 19369
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] [tools] Return immediately after the limit number of rows have been dumped
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] Return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19369/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/6//COMMIT_MSG@12
PS6, Line 12: due t
> nit: How about - "due to which there" ?
Done
http://gerrit.cloudera.org:8080/#/c/19369/6//COMMIT_MSG@15
PS6, Line 15: improve
> nit: improves
Done
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:
http://gerrit.cloudera.org:8080/#/c/19369/6/src/kudu/tablet/compaction.h@262
PS6, Line 262: is eq
> nit: "is equal"
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: 8
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 09:22:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tools] return immediately after the limit number of rows have been dumped
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19369 )
Change subject: [tools] return immediately after the limit number of rows have been dumped
......................................................................
Patch Set 5: Code-Review+1
(2 comments)
LGTM.
http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19369/2//COMMIT_MSG@8
PS2, Line 8:
: The --nrows flag for 'local_replica dump rowset'
: just limits the last output row co
> Thansk!
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:
> I meant it's able to early break instead of collect all rows and limit the
Yes. The patch has the improvement benefit.
If only fix the bug, what I said is enough.
LGTM.
--
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: 5
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Wang Xixu <14...@qq.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Thu, 12 Jan 2023 12:32:18 +0000
Gerrit-HasComments: Yes