You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Ádám Bakai (Code Review)" <ge...@cloudera.org> on 2022/10/28 13:50:08 UTC
[kudu-CR] KUDU-1698 Test RPC and session being separate.
Ádám Bakai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19175
Change subject: KUDU-1698 Test RPC and session being separate.
......................................................................
KUDU-1698 Test RPC and session being separate.
This test creates a scenario where the client reaches RPC timeout but not
session timeout. This way it verifies that RPC timeout is used by the
client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
---
M src/kudu/client/client-test.cc
1 file changed, 34 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/1
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 1
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#8).
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/8
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 8
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#9).
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/9
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 9
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Patch Set 10:
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/26837/ : FAILURE
The newly added test ClientTest.TestDefaultRPCTimeoutSessionTimeoutDifferent is still failing, at least in TSAN builds: http://jenkins.kudu.apache.org/job/kudu-gerrit/26837/BUILD_TYPE=TSAN/testReport/junit/(root)/ClientTest/TestDefaultRPCTimeoutSessionTimeoutDifferent/
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 10
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 17:32:42 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1698 Test that RPC and session timeout are separate entities
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#5).
Change subject: KUDU-1698 Test that RPC and session timeout are separate entities
......................................................................
KUDU-1698 Test that RPC and session timeout are separate entities
This test creates a scenario where the client reaches RPC timeout but not
session timeout. This way, it verifies that RPC timeout is used by the
client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 29 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/5
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 5
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] KUDU-1698 Test that RPC and session timeout being separate entities
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout being separate entities
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/19175/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19175/3//COMMIT_MSG@7
PS3, Line 7: being
nit: are
http://gerrit.cloudera.org:8080/#/c/19175/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19175/4//COMMIT_MSG@7
PS4, Line 7: being
nit: are
http://gerrit.cloudera.org:8080/#/c/19175/4//COMMIT_MSG@9
PS4, Line 9:
nit: comma
http://gerrit.cloudera.org:8080/#/c/19175/4//COMMIT_MSG@10
PS4, Line 10:
nit: comma
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc@912
PS4, Line 912: RPCTimeoutMs
nit: rpcTimeoutMs. Also, maybe make it constexpr?
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc@921
PS4, Line 921: succesful
nit: successful
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc@923
PS4, Line 923: MakeScopedCleanup
There is a SCOPED_CLEANUP macro, it should be okay to use it here as we don't want to cancel this thread.
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc@931
PS4, Line 931: cluster_->CreateClient(&builder, &client_);
Maybe wrap it in an ASSERT_OK?
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc@932
PS4, Line 932: auto ent = cluster_->mini_master()->master()->metric_entity();
Does this need to be created so early? Can we move it to just above where the metric is instantiated?
http://gerrit.cloudera.org:8080/#/c/19175/4/src/kudu/client/client-test.cc@935
PS4, Line 935: unique_ptr<KuduInsert> insert;
: insert = BuildTestInsert(table.get(), 2);
: ASSERT_OK(session->Apply(insert.release()));
can this be one line?
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 4
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Nov 2022 21:48:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1698 Test RPC and session timeout being separate entities
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#2).
Change subject: KUDU-1698 Test RPC and session timeout being separate entities
......................................................................
KUDU-1698 Test RPC and session timeout being separate entities
This test creates a scenario where the client reaches RPC timeout but not
session timeout. This way it verifies that RPC timeout is used by the
client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
---
M src/kudu/client/client-test.cc
1 file changed, 34 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/2
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 2
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Test RPC and session timeout independence
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#6).
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 40 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/6
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 6
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19175/10/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/10/src/kudu/client/client-test.cc@979
PS10, Line 979: shared_ptr<KuduTable> table;
I guess it makes sense to keep timeouts in the order of seconds to avoid flakiness, but not run this test unless KUDU_ALLOW_SLOW_TESTS is set.
https://gerrit.cloudera.org/#/c/19175/6/src/kudu/client/client-test.cc@915
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 10
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 17:34:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 11
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Dec 2022 16:16:50 +0000
Gerrit-HasComments: No
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@936
PS5, Line 936: }
> The problem is that I couldn't find any metric that works like that.
It seems TotalCount() for the request latency histrogram is good enough for that, right?
http://gerrit.cloudera.org:8080/#/c/19175/6/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/6/src/kudu/client/client-test.cc@915
PS6, Line 915:
The rule of thumb is: a test that takes 3 seconds or more is considered slow, so it should be skipped unless KUDU_ALLOW_SLOW_TESTS environment variable is set.
With that, please add SKIP_IF_SLOW_NOT_ALLOWED() in the very beginning of this test since it could run more than 10 seconds.
Is it possible to reduce the timeout settings not introducing a lot of flakiness?
http://gerrit.cloudera.org:8080/#/c/19175/6/src/kudu/client/client-test.cc@928
PS6, Line 928: const shared_ptr<KuduSession> session = client_->NewSession();
nit: I guess the cleanup should also call 'latch.CountDown();' before joining the threads to reduce waiting time if something failed
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 5
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Nov 2022 20:30:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1698 Test that RPC and session timeout are separate entities
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout are separate entities
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@936
PS5, Line 936: }
> BTW, checking for the exact number of requests might be a source of flakine
The problem is that I couldn't find any metric that works like that.
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 5
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 13:54:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1698 Test that RPC and session timeout are separate entities
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout are separate entities
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 5
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Nov 2022 07:12:39 +0000
Gerrit-HasComments: No
[kudu-CR] Test RPC and session timeout independence
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#10).
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/10
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 10
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
Reviewed-on: http://gerrit.cloudera.org:8080/19175
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/client/client-test.cc
1 file changed, 45 insertions(+), 0 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 12
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] KUDU-1698 Test that RPC and session timeout being separate entities
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#4).
Change subject: KUDU-1698 Test that RPC and session timeout being separate entities
......................................................................
KUDU-1698 Test that RPC and session timeout being separate entities
This test creates a scenario where the client reaches RPC timeout but not
session timeout. This way it verifies that RPC timeout is used by the
client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
---
M src/kudu/client/client-test.cc
1 file changed, 34 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/4
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 4
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] KUDU-1698 Test that RPC and session timeout being separate entities
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout being separate entities
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19175/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/3/src/kudu/client/client-test.cc@912
PS3, Line 912: RPCTimeoutMs
nit: this should be a const
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 3
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Oct 2022 15:23:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Patch Set 7:
It seems the new test is failing time to time:
http://jenkins.kudu.apache.org/job/kudu-gerrit/26822/BUILD_TYPE=TSAN/testReport/junit/(root)/ClientTest/TestDefaultRPCTimeoutSessionTimeoutDifferent/
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 7
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Dec 2022 17:45:00 +0000
Gerrit-HasComments: No
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: Test RPC and session timeout independence
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 11
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: Test RPC and session timeout independence
......................................................................
Removed -Verified by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 11
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] KUDU-1698 Test that RPC and session timeout are separate entities
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout are separate entities
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@913
PS5, Line 913: FLAGS_master_inject_latency_on_tablet_lookups_ms =
: rpcTimeoutMs * 2; // tablet lookup will trigger timeout
style nit for here and elsewhere with in-line comments: another way to format this might be
// This is to make tablet lookups timing out.
FLAGS_master_inject_latency_on_tablet_lookups_ms = rpcTimeoutMs * 2;
That way it's less line breaks within a single semantic scope/statement and more space for explanations.
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@930
PS5, Line 930: auto ent = cluster_->mini_master()->master()->metric_entity();
Shouldn't we check the metric value of GetTableLocations before issuing session->Apply call? Since the all the rest if a black box from this point, the preliminary code might do some calls in background (it's not so as of now, but the code in the test should be more explicit in that regard).
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@932
PS5, Line 932: ASSERT_EQ(METRIC_handler_latency_kudu_master_MasterService_GetTableLocations.Instantiate(ent)
It would be great to add a comment to explain why seeing that number this number of requests guarantees the timeouts for session and an RPC are different. It's not very obvious for a reader who has no enough context. Alternatively, you could add an explanation of the overall scenario just above the TEST_F line that introduces this scenario.
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@932
PS5, Line 932: ASSERT_EQ(METRIC_handler_latency_kudu_master_MasterService_GetTableLocations.Instantiate(ent)
: ->TotalCount(),
: 2); // check that there was a retry
nit: the expected value comes first in {ASSERT,EXPECT}_EQ() macros, otherwise it's hard to comprehend the output if the assertion triggers
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@935
PS5, Line 935: set_timeout_back_to_zero.join();
What if one of the assertions above trigger? Will the test crash because of destroying thread instance while it's not yet joined?
To fix this issue, consider using ScopedCleanup or the SCOPED_CLEANUP() macro. See https://github.com/apache/kudu/blob/10efaf2c77dfe5e4474505e0267c583c011703be/src/kudu/clock/hybrid_clock-test.cc#L326-L331 for an example.
http://gerrit.cloudera.org:8080/#/c/19175/5/src/kudu/client/client-test.cc@936
PS5, Line 936: }
BTW, checking for the exact number of requests might be a source of flakiness due to scheduler anomalies. Instead, I'd recommend checking that the number of registered requests after issuing Apply() is greater than the number of registered requests after Apply() succeeded.
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 5
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Nov 2022 21:32:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1698 Test that RPC and session timeout being separate entities
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#3).
Change subject: KUDU-1698 Test that RPC and session timeout being separate entities
......................................................................
KUDU-1698 Test that RPC and session timeout being separate entities
This test creates a scenario where the client reaches RPC timeout but not
session timeout. This way it verifies that RPC timeout is used by the
client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
---
M src/kudu/client/client-test.cc
1 file changed, 34 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/3
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 3
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Test RPC and session timeout independence
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: Test RPC and session timeout independence
......................................................................
Patch Set 11: Verified+1
unrelated test failure in RaftConsensusITest.TestSlowFollower
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 11
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Dec 2022 16:16:44 +0000
Gerrit-HasComments: No
[kudu-CR] Test RPC and session timeout independence
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#7).
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 44 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/7
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 7
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] Test RPC and session timeout independence
Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19175
to look at the new patch set (#11).
Change subject: Test RPC and session timeout independence
......................................................................
Test RPC and session timeout independence
This test creates a scenario where the client reaches RPC timeout but
not session timeout. This way, it verifies that RPC timeout is used by
the client.
Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Signed-off-by: Ádám Bakai <ab...@cloudera.com>
---
M src/kudu/client/client-test.cc
1 file changed, 45 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/19175/11
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 11
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
[kudu-CR] KUDU-1698 Test that RPC and session timeout are separate entities
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19175 )
Change subject: KUDU-1698 Test that RPC and session timeout are separate entities
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19175/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19175/5//COMMIT_MSG@9
PS5, Line 9: This test creates a scenario where the client reaches RPC timeout but not
: session timeout. This way, it verifies that RPC timeout is used by the
: client.
Please keep lines in the commit description under 72 symbols in length (gerrit shows the dotted line there). Overall, please check out this guide https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines that you could find referenced at https://kudu.apache.org/docs/contributing.html#_submitting_patches
--
To view, visit http://gerrit.cloudera.org:8080/19175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I177f5dcf04417edff8816690765a0c2b57e44036
Gerrit-Change-Number: 19175
Gerrit-PatchSet: 5
Gerrit-Owner: Ádám Bakai <ab...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Nov 2022 21:35:08 +0000
Gerrit-HasComments: Yes