You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/10/06 03:58:02 UTC

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution 

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................

[flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

In this test we're purposefully failing dns resolution and testing
that we get that error back from the client. However this is currently
flaky because we sometimes reach the deadline before getting the dns
error. I increased this to the double (1 sec). I'm reticent to increase
it further because, while we want the test to get the dns error, it will
keep trying until the deadline is reached, making a pretty long test
even longer.

Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
---
M src/kudu/client/client-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>

[kudu-CR] [flaky tests] Fix ClientTest.TestFailedDnsResolution

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [flaky tests] Fix ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 3: Code-Review+2

> Uploaded patch set 3.

It's good enough for a stopgap solution, but we need to fix it properly.  I hope we can address this as part of work in the context of diagnosing client-side timeouts.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 1:

> Uploaded patch set 1.

Did it help to reduce failure rate at all?  I.e. what is the failure ration if running it under TSAN via dist_test.py?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4643/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS2, Line 1937: string
nit: 'static const string' or at least 'const string' to express this variable is a reference and isn't changing at all.


PS2, Line 1938: ==
Should it be '!=' instead?


PS2, Line 1939: ASSERT_TRUE
nit: consider using ASSERT_LE() instead of ASSERT_TRUE()


PS2, Line 1939: dns
nit: DNS


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................

[flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

In this test we're testing that we get dns resolution error back when
a dns resolution error fails. However there is a narrow window in which
the returned error might be a master GetTabletLocations() RPC instead,
because of KUDU-1466.

This changes the test to retry until it gets the appropriate errors.

Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
---
M src/kudu/client/client-test.cc
1 file changed, 22 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [flaky tests] Fix ClientTest.TestFailedDnsResolution

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: [flaky tests] Fix ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 3: Verified+1

observables test flakyness

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [flaky tests] Fix ClientTest.TestFailedDnsResolution

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: [flaky tests] Fix ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4643/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS2, Line 1937: string
> nit: 'static const string' or at least 'const string' to express this varia
moved this outside the loop.


PS2, Line 1938: ==
> Should it be '!=' instead?
oops, good catch. as you probably can figure, I wasn't actually able to run these locally.


PS2, Line 1939: ASSERT_TRUE
> nit: consider using ASSERT_LE() instead of ASSERT_TRUE()
Done


PS2, Line 1939: dns
> nit: DNS
we use dns everywhere else


Line 1945:     }
> break is missing here: if it's the desired error condition, it's necessary 
oops, good catch


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4643/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 1945:     }
break is missing here: if it's the desired error condition, it's necessary to break out of the retry cycle.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 1:

Based on my previous investigation of this test, I dont think it's just a matter of the timeout. See https://gerrit.cloudera.org/#/c/3326/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: [flaky tests] Increase the timeout in ClientTest.TestFailedDnsResolution
......................................................................


Patch Set 2:

thanks for pointing that out. changed the strategy to simply ignoring that error and trying again.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [flaky tests] Fix ClientTest.TestFailedDnsResolution

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.

Change subject: [flaky tests] Fix ClientTest.TestFailedDnsResolution
......................................................................


[flaky tests] Fix ClientTest.TestFailedDnsResolution

In this test we're testing that we get dns resolution error back when
a dns resolution error fails. However there is a narrow window in which
the returned error might be a master GetTabletLocations() RPC instead,
because of KUDU-1466.

This changes the test to retry until it gets the appropriate error.

Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Reviewed-on: http://gerrit.cloudera.org:8080/4643
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/client/client-test.cc
1 file changed, 24 insertions(+), 10 deletions(-)

Approvals:
  David Ribeiro Alves: Verified
  Alexey Serbin: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [flaky tests] Fix ClientTest.TestFailedDnsResolution

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: [flaky tests] Fix ClientTest.TestFailedDnsResolution
......................................................................

[flaky tests] Fix ClientTest.TestFailedDnsResolution

In this test we're testing that we get dns resolution error back when
a dns resolution error fails. However there is a narrow window in which
the returned error might be a master GetTabletLocations() RPC instead,
because of KUDU-1466.

This changes the test to retry until it gets the appropriate error.

Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
---
M src/kudu/client/client-test.cc
1 file changed, 24 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fa3ceb7215d60cd23efabf729731254524e1625
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>