You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/10/31 08:24:30 UTC
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16687
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
[client] KUDU-2966 configure connection negotiation timeout
This patch addresses KUDU-2966 for the Kudu C++ client, making
the timeout for the connection negotiation configurable. This patch
also contains a unit test for the newly introduced functionality.
As for the motivation for this patch: we have observed a few clusters
in the wild where connection negotiations were a bit slow when servers
are under heavy load. It would be nice to control the connection
negotiation timeout at the client side as well, so at least kudu CLI
tool could use this functionality.
The newly introduced functionality is to be used at least in kudu CLI
(will be posted in a separate follow-up patch).
Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
8 files changed, 111 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/16687/1
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16687 )
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Nov 2020 04:45:58 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16687
to look at the new patch set (#2).
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
[client] KUDU-2966 configure connection negotiation timeout
This patch addresses KUDU-2966 for the Kudu C++ client, making
the timeout for the connection negotiation configurable. This patch
also contains a unit test for the newly introduced functionality.
As for the motivation for this patch: we have observed a few clusters
in the wild where connection negotiations were a bit slow when servers
are under heavy load. It would be nice to control the connection
negotiation timeout at the client side as well, so at least kudu CLI
tool could use this functionality.
The newly introduced functionality is to be used at least in kudu CLI
(will be posted in a separate follow-up patch).
Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
8 files changed, 111 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/16687/2
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16687 )
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Patch Set 2: Verified+1
unrelated test failure in TxnStatusTableITest.TestSystemClientTServerDown
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Nov 2020 04:06:03 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16687 )
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
[client] KUDU-2966 configure connection negotiation timeout
This patch addresses KUDU-2966 for the Kudu C++ client, making
the timeout for the connection negotiation configurable. This patch
also contains a unit test for the newly introduced functionality.
As for the motivation for this patch: we have observed a few clusters
in the wild where connection negotiations were a bit slow when servers
are under heavy load. It would be nice to control the connection
negotiation timeout at the client side as well, so at least kudu CLI
tool could use this functionality.
The newly introduced functionality is to be used at least in kudu CLI
(will be posted in a separate follow-up patch).
Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Reviewed-on: http://gerrit.cloudera.org:8080/16687
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
8 files changed, 111 insertions(+), 34 deletions(-)
Approvals:
Alexey Serbin: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16687 )
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Patch Set 1: Verified+1
Unrelated test failures covered by the following JIRAs:
* https://issues.apache.org/jira/browse/KUDU-2501
* https://issues.apache.org/jira/browse/KUDU-1552
* https://issues.apache.org/jira/browse/KUDU-2109
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 31 Oct 2020 16:36:38 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16687 )
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc@898
PS1, Line 898: auto s = b.Build(&c);
: ASSERT_OK(s);
nit: inline the ASSERT? Same below
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc@911
PS1, Line 911: timeout
nit: kConstNamingScheme?
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc@919
PS1, Line 919: Initialized
nit: isn't this taken care of by the comparison?
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client.h@268
PS1, Line 268: If not provided, the underlying messenger is created with reasonable
: /// default. The result value could be retrieved using
: /// @c KuduClient.connection_negotiation_timeout() after an instance of
: /// @c KuduClient is created.
nit: Would it make sense to add a small blurb about what connection negotiation is, and why it might be slow?
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 03 Nov 2020 05:30:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-2966 configure connection negotiation timeout
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16687 )
Change subject: [client] KUDU-2966 configure connection negotiation timeout
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc@898
PS1, Line 898: auto s = b.Build(&c);
: ASSERT_OK(s);
> nit: inline the ASSERT? Same below
Done
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc@911
PS1, Line 911: timeout
> nit: kConstNamingScheme?
Done
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client-test.cc@919
PS1, Line 919: Initialized
> nit: isn't this taken care of by the comparison?
Yes, but those comparison operators have DCHECK(Initialized()) in their body, so I was thinking avoid a crash in DEBUG configuration if this ever regresses. From the other side, crash would certainly point to the issue as well :)
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/16687/1/src/kudu/client/client.h@268
PS1, Line 268: If not provided, the underlying messenger is created with reasonable
: /// default. The result value could be retrieved using
: /// @c KuduClient.connection_negotiation_timeout() after an instance of
: /// @c KuduClient is created.
> nit: Would it make sense to add a small blurb about what connection negotia
Done
--
To view, visit http://gerrit.cloudera.org:8080/16687
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8573a572887be041637e28e518a7cd3a6d1f1693
Gerrit-Change-Number: 16687
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 04 Nov 2020 02:24:10 +0000
Gerrit-HasComments: Yes