You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "zhangqianqiong (Code Review)" <ge...@cloudera.org> on 2018/08/02 10:51:36 UTC
[kudu-CR] KUDU-2525: fix a bug for kudu: scanner return the zero rows
zhangqianqiong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11111
Change subject: KUDU-2525: fix a bug for kudu: scanner return the zero rows
......................................................................
KUDU-2525: fix a bug for kudu: scanner return the zero rows
Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11111/1
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 1
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11111/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@447
PS3, Line 447: try {
Some other cosmetic suggestions:
1. Rewrap the comment to adhere to Kudu's 80 character soft limit (https://kudu.apache.org/docs/contributing.html#_line_length)
2. There is whitespace at the end of each row; gerrit flagged this with red.
3. The convention for TODO comments is to include an appropriate JIRA if there is one, or just your name if not. So "TODO(qqzhang): is better than just "TODO:".
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 3
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Mon, 06 Aug 2018 22:19:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Reviewed-on: http://gerrit.cloudera.org:8080/11111
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 10 insertions(+), 1 deletion(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 5
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "zhangqianqiong (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11111
to look at the new patch set (#3).
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 9 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11111/3
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 3
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "zhangqianqiong (Code Review)" <ge...@cloudera.org>.
zhangqianqiong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@66
PS1, Line 66: * This input format generates one split per tablet and the only location for each split is that
> Could you try to synthesize this into a unit test? That'll help solidify th
I also thought It is OK in here without a unit test.
If possible, The TabletService.Scan() of the backend should add a unit test in case the buget_ms is out.
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@448
PS1, Line 448: // TODO(qqzhang) scanner.nextRows() sometimes returns an empty RowResultIterator,
> Thanks for the added content, but why TODO(...)? Again, we only use TODOs w
Thanks for your point, I re-edited the comments
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 2
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Sat, 04 Aug 2018 02:35:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: fix a bug for kudu: scanner return the zero rows
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: fix a bug for kudu: scanner return the zero rows
......................................................................
Patch Set 1:
(3 comments)
Could you reproduce the bug in a kudu-mapreduce unit test?
http://gerrit.cloudera.org:8080/#/c/11111/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11111/1//COMMIT_MSG@7
PS1, Line 7: KUDU-2525: fix a bug for kudu: scanner return the zero rows
Rewrite as:
KUDU-2525: KuduTableInputFormat may halt before exhausting scan
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@66
PS1, Line 66: * This input format generates one split per tablet and the only location for each split is that
Given that there's one split per tablet, how does this bug manifest? I thought the only time scanner.nextRows() returned an empty RowResultIterator was when the scan ended on one tablet and started up on the next, and the next tablet didn't have any rows.
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@448
PS1, Line 448: // FIXME(qqzhang) the loop is only for the responded empty rows
Typically we mark things like this with TODO(...); what's left to do here?
Could you instead modify this comment to make it more instructive? You can explain how scanner.nextRows() is allowed to return an empty RowResultIterator, but until scanner.hasMoreRows() returns false, we need to continue iterating on the scanner.
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 1
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 02 Aug 2018 18:07:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@66
PS1, Line 66: * This input format generates one split per tablet and the only location for each split is that
> Could you try to synthesize this into a unit test? That'll help solidify th
Empty scan batches can happen in at least two ways:
* It's possible for the client to set the scan bytes limit to 0 when creating a new scanner, and then use something bigger for subsequent continue scan calls. In the case of the new scanner call it will be an empty row batch (see https://github.com/apache/kudu/blob/master/src/kudu/tserver/tablet_service.cc?utf8=%E2%9C%93#L1867). As far as I know you can't actually configure a Java scanner to do this, though.
* If a scan is filtering many rows it may not be able to find a single matching row before the internal 500ms timeout expires. Unfortunately this is one the few completely hardcoded timeouts in Kudu, so we can't easily toggle it for a unit test: https://github.com/apache/kudu/blob/master/src/kudu/tserver/tablet_service.cc?utf8=%E2%9C%93#L1941
Neither of these are easy to reproduce in a unit tests easily. My best idea would be to add a test-only flag to the tserver to 'inject' empty batches randomly.
That being said, because the Spark KuduRDD has effectively the exact same loop (https://github.com/apache/kudu/blob/master/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala#L124), I'd also be OK just landing this as-is without a unit test.
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 2
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Fri, 03 Aug 2018 18:09:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "zhangqianqiong (Code Review)" <ge...@cloudera.org>.
zhangqianqiong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11111/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@447
PS3, Line 447: try {
> Some other cosmetic suggestions:
Thank you for your suggestions.
For point 1, (https://kudu.apache.org/docs/contributing.html#_line_length) indicates that line lengths of 100 characters per line is allowed. But I still have limited it to 80 characters.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 3
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Aug 2018 02:34:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "zhangqianqiong (Code Review)" <ge...@cloudera.org>.
zhangqianqiong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11111/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11111/1//COMMIT_MSG@7
PS1, Line 7: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
> Rewrite as:
Done
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@66
PS1, Line 66: * This input format generates one split per tablet and the only location for each split is that
> Given that there's one split per tablet, how does this bug manifest? I thou
I can be sure that there is not an empty tablet in the table.
scanner.nextRows() sometimes returned an empty RowResultIterator, and sometimes returned a normal RowResultIterator. In other words, the results is random. So, I guess that the scan is too time-consuming to timeout and returned the empty RowResultIerator
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@448
PS1, Line 448: // TODO(qqzhang) scanner.nextRows() sometimes returns an empty RowResultIterator,
> Typically we mark things like this with TODO(...); what's left to do here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 2
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Fri, 03 Aug 2018 02:45:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "zhangqianqiong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11111
to look at the new patch set (#2).
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11111/2
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 2
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11111/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@447
PS3, Line 447: try {
> Thank you for your suggestions.
Just to make sure we're on the same page:
The Kudu team allows line lengths of 100 characters per line, rather than Google’s standard of 80. Try to keep under 80 where possible, but you can spill over to 100 or so if necessary.
You're technically correct that up to 100 is OK, but we do try to stay under 80 where possible (which I referred to as a "soft limit").
In any case, thank you for making the change.
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 4
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Aug 2018 02:49:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "zhangqianqiong (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11111
to look at the new patch set (#4).
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
---
M java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11111/4
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 4
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
[kudu-CR] KUDU-2525: KuduTableInputFormat may halt before exhausting scan
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11111 )
Change subject: KUDU-2525: KuduTableInputFormat may halt before exhausting scan
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@66
PS1, Line 66: * This input format generates one split per tablet and the only location for each split is that
> I can be sure that there is not an empty tablet in the table.
Could you try to synthesize this into a unit test? That'll help solidify the conditions that must be met for this to occur, and will also serve as a regression test.
http://gerrit.cloudera.org:8080/#/c/11111/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@448
PS1, Line 448: // TODO(qqzhang) scanner.nextRows() sometimes returns an empty RowResultIterator,
> Done
Thanks for the added content, but why TODO(...)? Again, we only use TODOs when there's some work left to be done.
See this Google Style Guide link for more details: https://google.github.io/styleguide/cppguide.html#TODO_Comments
--
To view, visit http://gerrit.cloudera.org:8080/11111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbfdd2efbd281e4d849917664b33e183e180bafd
Gerrit-Change-Number: 11111
Gerrit-PatchSet: 2
Gerrit-Owner: zhangqianqiong <qq...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: zhangqianqiong <qq...@gmail.com>
Gerrit-Comment-Date: Fri, 03 Aug 2018 16:49:02 +0000
Gerrit-HasComments: Yes