You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/03/21 01:10:23 UTC

[kudu-CR] KUDU-2687: another attempt at fixing ITClient retries 

Hello Will Berkeley, Mike Percy,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/12820

to review the following change.


Change subject: KUDU-2687: another attempt at fixing ITClient retries
......................................................................

KUDU-2687: another attempt at fixing ITClient retries

Perhaps they're broken because, when the test fails, we never join the
test threads? This patch addresses that by reworking the latch usage. It
also removes one of the latches; I don't think both were needed.

Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
---
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
1 file changed, 23 insertions(+), 17 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/12820/1
-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2687: another attempt at fixing ITClient retries

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12820 )

Change subject: KUDU-2687: another attempt at fixing ITClient retries
......................................................................


Patch Set 1: Verified-1

I'm injecting failures myself and the retrying is _still_ broken. Hang on...


-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Mar 2019 03:34:34 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12820 )

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12820/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java:

http://gerrit.cloudera.org:8080/#/c/12820/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java@117
PS2, Line 117:       keepRunningLatch.countDown();
> oh, nm I see it `reportError` counts the latch down. Could you add a commen
Doesn't the comment on L112-113 indicate that if there's an error, the latch counted down?

In any case, the CountDownLatch.await() docs say that if it returns true, the latch has reached a count of 0. Meaning, the branch here isn't because the test is doing something special; it's just the natural way CountDownLatches are used.



-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Mar 2019 18:45:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12820

to look at the new patch set (#3).

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................

KUDU-2687: fix ITClient retries (take two)

After adding randomly injected failures, I finally figured out why ITClient
retries were broken: because, surprisingly, the RetryRule does _not_
reinitialize non-static test class fields. As such, the latches' counts were
never reset, and so every test retry would immediately fail.

Along the way I improved the test in a few ways:
- Switched from two latches to just one.
- Ensured that the test cleaned up its threads on failure.
- Made the test throw the failing exception directly rather than logging it
  and forcing you to scroll around looking for the logged message.
- Switched from Random to RandomUtils.
- Relatedly, fixed the reuse of CapturingLogAppender in TestSecurity, which
  caused every test to capture the events of all tests that ran before it.

Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
---
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
2 files changed, 41 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/12820/3
-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12820 )

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................

KUDU-2687: fix ITClient retries (take two)

After adding randomly injected failures, I finally figured out why ITClient
retries were broken: because, surprisingly, the RetryRule does _not_
reinitialize non-static test class fields. As such, the latches' counts were
never reset, and so every test retry would immediately fail.

Along the way I improved the test in a few ways:
- Switched from two latches to just one.
- Ensured that the test cleaned up its threads on failure.
- Made the test throw the failing exception directly rather than logging it
  and forcing you to scroll around looking for the logged message.
- Switched from Random to RandomUtils.
- Relatedly, fixed the reuse of CapturingLogAppender in TestSecurity, which
  caused every test to capture the events of all tests that ran before it.

Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Reviewed-on: http://gerrit.cloudera.org:8080/12820
Reviewed-by: Grant Henke <gr...@apache.org>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
2 files changed, 41 insertions(+), 26 deletions(-)

Approvals:
  Grant Henke: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/12820

to look at the new patch set (#2).

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................

KUDU-2687: fix ITClient retries (take two)

After adding randomly injected failures, I finally figured out why ITClient
retries were broken: because, surprisingly, the RetryRule does _not_
reinitialize non-static test class fields. As such, the latches' counts were
never reset, and so every test retry would immediately fail.

Along the way I improved the test in a few ways:
- Switched from two latches to just one.
- Ensured that the test cleaned up its threads on failure.
- Made the test throw the failing exception directly rather than logging it
  and forcing you to scroll around looking for the logged message.

Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
---
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
1 file changed, 35 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/12820/2
-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12820 )

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12820/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java:

http://gerrit.cloudera.org:8080/#/c/12820/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java@117
PS2, Line 117:       keepRunningLatch.countDown();
Why don't we wan't to stop the threads in failure?



-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Mar 2019 15:43:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12820 )

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Mar 2019 20:33:55 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2687: fix ITClient retries (take two)

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12820 )

Change subject: KUDU-2687: fix ITClient retries (take two)
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12820/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java:

http://gerrit.cloudera.org:8080/#/c/12820/2/java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java@117
PS2, Line 117:       keepRunningLatch.countDown();
> Why don't we wan't to stop the threads in failure?
oh, nm I see it `reportError` counts the latch down. Could you add a comment here to make that obvious.



-- 
To view, visit http://gerrit.cloudera.org:8080/12820
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31ec2d708d0ed41f25abcf09011967062c9a56c6
Gerrit-Change-Number: 12820
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 21 Mar 2019 15:44:44 +0000
Gerrit-HasComments: Yes