[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13702 )

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..


Patch Set 4:

Can some committers of Kudu please merge this patch for me ? I cannot merge it 
myself. Thanks.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 27 Jun 2019 22:34:03 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13702 )

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..


Patch Set 4:

(5 comments)

I tested with this patch with TLS and Kerberos enabled:

client connection to 172.31.115.178:27000 recv error: Network error: failed to 
read from TLS socket (remote: 0.0.0.0:0): Connection reset by peer (error 104)

The connection was closed to the peer was closed shortly after the iptables 
rule was installed. Confirmed that a new connection was re-created afterwards.

http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:

http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/rpc/reactor.cc@631
PS3, Line 631: Status keepalive_status = conn->SetTcpKeepAlive(
> nit: consider using scope-only variable here, something like
Done


http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/rpc/rpc-test.cc@790
PS3, Line 790:   // Set up server.
> As Todd already mentioned, it would be great to have a scenario when connec
There is no easy way to simulate this situation programmatically without using 
iptables or tc qdisc netem command.

I manually tested with iptables rules with and without TLS by applying this 
patch in Impala code base.


http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/rpc/rpc-test.cc@798
PS3, Line 798:   FLAGS_tcp_keepalive_retry_count = 1;
> no need for this -- our toplevel KuduTest base class handles this for all t
Done


http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:

http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/util/net/socket.h@160
PS3, Line 160:
> nit: it's a bit strange to see MonoDelta here, supposedly lower-level than
Done


http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:

http://gerrit.cloudera.org:8080/#/c/13702/3/src/kudu/util/net/socket.cc@591
PS3, Line 591: DCHECK_GT(id
> nit: static const char* const ?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 25 Jun 2019 21:27:03 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..

KUDU-2192: Enable TCP keepalive for all outbound connections

This change enables TCP keepalive for all outbound connections.
This aims to handle cases in which the remote peer may have
dropped off the network without sending a TCP RST. For instance,
a remote host could have hit a kernel panic and got power cycled.
In which case, the existing TCP connection to that host may be
stale. In an idle cluster, this stale connection may not be detected
until the next use of it, in which case it will result in a RPC
failure due to TCP RST sent from the restarted peer.

By enabling TCP keepalive, we ensure that stale TCP connections
in an idle cluster will be detected and closed within a time bound
so a new connection will be created on the next use. This change
introduces 3 different flags:

--tcp_keepalive_probe_period_s: the duration in seconds a TCP connection
has to be idle before keepalive probes started to be sent.

--tcp_keepalive_retry_period_s: the duration in seconds between successive
keepalive probes if previous probes didn't get an ACK from remote peer.

--tcp_keepalive_retry_count: the maximum number of TCP keepalive probes
sent without an ACK before declaring the remote peer as dead.

Testing:
- Used TCP dump to verify that keepalive probes are being sent periodically.
- Verified that blocking all incoming traffic to a server's port via an iptable
rule caused the TCP connection to be closed and the keepalive probes to stop
eventually.

Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
7 files changed, 112 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/13702/4
--
To view, visit http://gerrit.cloudera.org:8080/13702
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13702 )

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..


Patch Set 3:

The server shutdown test case in rpc-test is a good one but just wondering if 
there are other similar types of test in the test suite which I can extend to 
test this patch ?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 21 Jun 2019 17:31:18 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13702 )

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..


Patch Set 3:

I mostly tested this change using iptables in the context of Impala's use of 
KRPC and I was able to get error message like:

connection.cc:518] client connection to 127.0.0.1:27000 recv error: Network 
error: recv error from 0.0.0.0:0: Transport endpoint is not connected (error 
107)

I believe there are probably more paths in Kudu which I haven't tried out yet 
but I am not too familiar with Kudu so any advice on which area to look.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 21 Jun 2019 17:29:50 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

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

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

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

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..

KUDU-2192: Enable TCP keepalive for all outbound connections

This change enables TCP keepalive for all outbound connections.
This aims to handle cases in which the remote peer may have
dropped off the network without sending a TCP RST. For instance,
a remote host could have hit a kernel panic and got power cycled.
In which case, the existing TCP connection to that host may be
stale. In an idle cluster, this stale connection may not be detected
until the next use of it, in which case it will result in a RPC
failure due to TCP RST sent from the restarted peer.

By enabling TCP keepalive, we ensure that stale TCP connections
in an idle cluster will be detected and closed within a time bound
so a new connection will be created on the next use. This change
introduces 3 different flags:

--tcp_keepalive_probe_period_s: the duration in seconds a TCP connection
has to be idle before keepalive probes started to be sent.

--tcp_keepalive_retry_period_s: the duration in seconds between successive
keepalive probes if previous probes didn't get an ACK from remote peer.

--tcp_keepalive_retry_count: the maximum number of TCP keepalive probes
sent without an ACK before declaring the remote peer as dead.

Testing:
- Used TCP dump to verify that keepalive probes are being sent periodically.
- Verified that blocking all incoming traffic to a server's port via an iptable
rule caused the TCP connection to be closed and the keepalive probes to stop
eventually.

Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
7 files changed, 112 insertions(+), 2 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..

KUDU-2192: Enable TCP keepalive for all outbound connections

This change enables TCP keepalive for all outbound connections.
This aims to handle cases in which the remote peer may have
dropped off the network without sending a TCP RST. For instance,
a remote host could have hit a kernel panic and got power cycled.
In which case, the existing TCP connection to that host may be
stale. In an idle cluster, this stale connection may not be detected
until the next use of it, in which case it will result in a RPC
failure due to TCP RST sent from the restarted peer.

By enabling TCP keepalive, we ensure that stale TCP connections
in an idle cluster will be detected and closed within a time bound
so a new connection will be created on the next use. This change
introduces 3 different flags:

--tcp_keepalive_probe_period_s: the duration in seconds a TCP connection
has to be idle before keepalive probes started to be sent.

--tcp_keepalive_retry_period_s: the duration in seconds between successive
keepalive probes if previous probes didn't get an ACK from remote peer.

--tcp_keepalive_retry_count: the maximum number of TCP keepalive probes
sent without an ACK before declaring the remote peer as dead.

Testing:
- Used TCP dump to verify that keepalive probes are being sent periodically.
- Verified that blocking all incoming traffic to a server's port via an iptable
rule caused the TCP connection to be closed and the keepalive probes to stop
eventually.

Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
7 files changed, 111 insertions(+), 2 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2192: Enable TCP keepalive for all outbound connections

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/13702


Change subject: KUDU-2192: Enable TCP keepalive for all outbound connections
..

KUDU-2192: Enable TCP keepalive for all outbound connections

This change enables TCP keepalive for all outbound connections.
This aims to handle cases in which the remote peer may have
dropped off the network without sending a TCP RST. For instance,
a remote host could have hit a kernel panic and got power cycled.
In which case, the existing TCP connection to that host may be
stale. In an idle cluster, this stale connection may not be detected
until the next use of it, in which case it will result in a RPC
failure due to TCP RST sent from the restarted peer.

By enabling TCP keepalive, we ensure that stale TCP connections
in an idle cluster will be detected and closed within a time bound
so a new connection will be created on the next use. This change
introduces 3 different flags:

--tcp_keepalive_probe_period_s: the duration in seconds a TCP connection
has to be idle before keepalive probes started to be sent.

--tcp_keepalive_retry_period_s: the duration in seconds between successive
keepalive probes if previous probes didn't get an ACK from remote peer.

--tcp_keepalive_retry_count: the maximum number of TCP keepalive probes
sent without an ACK before declaring the remote peer as dead.

Testing:
- Used TCP dump to verify that keepalive probes are being sent periodically.
- Verified that blocking all incoming traffic to a server's port via an iptable
rule caused the TCP connection to be closed and the keepalive probes to stop
eventually.

Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
---
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
5 files changed, 90 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa1d66d83aea1cc82d07fc6217be5fc1306695bc
Gerrit-Change-Number: 13702
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2706: Work around the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12545 )

Change subject: KUDU-2706: Work around the lack of thread safety in 
krb5_parse_name()
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12545/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12545/2//COMMIT_MSG@26
PS2, Line 26: Kerberos documentation
> nit: it would be nice if a reference to the doc were incorporated into the
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 21 Feb 2019 19:58:58 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2706: Work around the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2706: Work around the lack of thread safety in 
krb5_parse_name()
..

KUDU-2706: Work around the lack of thread safety in krb5_parse_name()

krb5_init_context() sets the field 'default_realm' in a krb5_context
object to 0. Upon first call to krb5_parse_name() with a principal
without realm specified (e.g. foo/bar), 'default_realm' in the
krb5_context object is lazily initialized.

When more than one negotiation threads are configured, it's possible
for multiple threads to call CanonicalizeKrb5Principal() in parallel.
CanonicalizeKrb5Principal() in turn calls krb5_parse_name(g_krb5_ctx, ...)
with no lock held. In addition, krb5_parse_name() is not thread safe as
it lazily initializes 'context->default_realm' without holding lock.
Consequently, 'g_krb5_ctx' which is shared and not supposed to be modified
after initialization may be inadvertently modified concurrently by multiple
threads, leading to crashes (e.g. double free) or errors.

This change works around the problem by initializing 'g_krb5_ctx->default_realm'
once in InitKrb5Ctx() by calling krb5_get_default_realm().

TODO: Fix unsafe sharing of 'g_krb5_ctx'. According to Kerberos documentation
(https://github.com/krb5/krb5/blob/master/doc/threads.txt), any use of 
krb5_context
must be confined to one thread at a time by the application code. The current
sharing of 'g_krb5_ctx' between threads without synchronization is in fact 
unsafe.

Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
---
M src/kudu/security/init.cc
1 file changed, 12 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/12545/4
--
To view, visit http://gerrit.cloudera.org:8080/12545
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2706: Work around the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2706: Work around the lack of thread safety in 
krb5_parse_name()
..

KUDU-2706: Work around the lack of thread safety in krb5_parse_name()

krb5_init_context() sets the field 'default_realm' in a krb5_context
object to 0. Upon first call to krb5_parse_name() with a principal
without realm specified (e.g. foo/bar), 'default_realm' in the
krb5_context object is lazily initialized.

When more than one negotiation threads are configured, it's possible
for multiple threads to call CanonicalizeKrb5Principal() in parallel.
CanonicalizeKrb5Principal() in turn calls krb5_parse_name(g_krb5_ctx, ...)
with no lock held. In addition, krb5_parse_name() is not thread safe as
it lazily initializes 'context->default_realm' without holding lock.
Consequently, 'g_krb5_ctx' which is shared and not supposed to be modified
after initialization may be inadvertently modified concurrently by multiple
threads, leading to crashes (e.g. double free) or errors.

This change works around the problem by initializing 'g_krb5_ctx->default_realm'
once in InitKrb5Ctx() by calling krb5_get_default_realm().

TODO: Fix unsafe sharing of 'g_krb5_ctx'. According to Kerberos documentation,
any use of krb5_context must be confined to one thread at a time by the
application code. The current sharing of 'g_krb5_ctx' between threads without
synchronization is in fact unsafe.

Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
---
M src/kudu/security/init.cc
1 file changed, 11 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2706: Work around the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12545 )

Change subject: KUDU-2706: Work around the lack of thread safety in 
krb5_parse_name()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12545/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/12545/2/src/kudu/security/init.cc@166
PS2, Line 166:   // TODO(KUDU-2706): Fix unsafe sharing of 'g_krb5_ctx'.
> warning: missing username/bug in TODO [google-readability-todo]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 21 Feb 2019 19:53:56 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2706: Work around the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2706: Work around the lack of thread safety in 
krb5_parse_name()
..

KUDU-2706: Work around the lack of thread safety in krb5_parse_name()

krb5_init_context() sets the field 'default_realm' in a krb5_context
object to 0. Upon first call to krb5_parse_name() with a principal
without realm specified (e.g. foo/bar), 'default_realm' in the
krb5_context object is lazily initialized.

When more than one negotiation threads are configured, it's possible
for multiple threads to call CanonicalizeKrb5Principal() in parallel.
CanonicalizeKrb5Principal() in turn calls krb5_parse_name(g_krb5_ctx, ...)
with no lock held. In addition, krb5_parse_name() is not thread safe as
it lazily initializes 'context->default_realm' without holding lock.
Consequently, 'g_krb5_ctx' which is shared and not supposed to be modified
after initialization may be inadvertently modified concurrently by multiple
threads, leading to crashes (e.g. double free) or errors.

This change works around the problem by initializing 'g_krb5_ctx->default_realm'
once in InitKrb5Ctx() by calling krb5_get_default_realm().

TODO: Fix unsafe sharing of 'g_krb5_ctx'. According to Kerberos documentation,
any use of krb5_context must be confined to one thread at a time by the
application code. The current sharing of 'g_krb5_ctx' between threads without
synchronization is in fact unsafe.

Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
---
M src/kudu/security/init.cc
1 file changed, 11 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2706: Work around the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12545 )

Change subject: KUDU-2706: Work around the lack of thread safety in 
krb5_parse_name()
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12545/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12545/1//COMMIT_MSG@17
PS1, Line 17: In addition, krb5_parse_name() is not thread safe as
> I think it's clearer to explain that krb5_parse_name isn't thread-safe beca
Done


http://gerrit.cloudera.org:8080/#/c/12545/1//COMMIT_MSG@20
PS1, Line 20: tialization may be inadvertently modified concu
> BTW, krb5_set_error_message() is also not thread safe.  However, if you are
Good point. That said, based on the stack trace in KUDU-2706, the workaround 
seems sufficient. However, as you pointed out, in case we hit any error in 
Kerberos library, krb5_set_error_message() may lead to crash due to unsafe 
sharing of 'g_krb5_ctx'. I cannot think of a easier workaround other than 
addressing the unsafe use of 'g_krb5_ctx' directly.


http://gerrit.cloudera.org:8080/#/c/12545/1//COMMIT_MSG@22
PS1, Line 22:
> it's not really explicit -- you're implicitly initializing it by forcing kr
Removed the word 'explicit'.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 21 Feb 2019 19:48:31 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2706: Workaround the lack of thread safety in krb5 parse name()

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/12545


Change subject: KUDU-2706: Workaround the lack of thread safety in 
krb5_parse_name()
..

KUDU-2706: Workaround the lack of thread safety in krb5_parse_name()

krb5_init_context() sets the field 'default_realm' in a krb5_context
object to 0. Upon first call to krb5_parse_name() with a principal
without realm specified (e.g. foo/bar), 'default_realm' in the
krb5_context object is lazily initialized.

When more than one negotiation threads are configured, it's possible
for multiple threads to call CanonicalizeKrb5Principal() in parallel.
CanonicalizeKrb5Principal() in turn calls krb5_parse_name(g_krb5_ctx, ...)
with no lock held. In addition, krb5_parse_name() is also not thread safe.
Consequently, 'g_krb5_ctx' which is shared and not supposed to be modified
after initialization may be inadvertently modified concurrently by multiple
threads, leading to crashes (e.g. double free) or errors.

This change works around the problem by explicitly initializing
'g_krb5_ctx->default_realm' once in InitKrb5Ctx() by calling
krb5_get_default_realm().

Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
---
M src/kudu/security/init.cc
1 file changed, 6 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1bf9224516e2996f51f319088179727f76741ebe
Gerrit-Change-Number: 12545
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] Add "network plane" as part of ConnectionId

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,

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

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

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

Change subject: Add "network_plane" as part of ConnectionId
..

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is not set.
A user can change it to a different value by calling
Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
7 files changed, 138 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/11681/5
--
To view, visit http://gerrit.cloudera.org:8080/11681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add "network plane" as part of ConnectionId

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon,

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

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

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

Change subject: Add "network_plane" as part of ConnectionId
..

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is set to
"default_network_plane". A user can change it to a different
value by calling Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 106 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/11681/4
--
To view, visit http://gerrit.cloudera.org:8080/11681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add "network plane" as part of ConnectionId

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon,

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

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

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

Change subject: Add "network_plane" as part of ConnectionId
..

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is set to
"default_network_plane". A user can change it to a non-default
value by calling Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 102 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add "network plane" as part of ConnectionId

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> Thanks for the idea. Will look into it.
Sorry for the delay. I adopted option 1. Thanks.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 06 Nov 2018 02:13:36 +
Gerrit-HasComments: Yes

[kudu-CR] Add "service name" as part of ConnectionId

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "service_name" as part of ConnectionId
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> I think this needs to be a separately-configured option. In Kudu we use lot
Thanks for the idea. Will look into it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 17 Oct 2018 00:43:18 +
Gerrit-HasComments: Yes

[kudu-CR] Add "service name" as part of ConnectionId

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon,

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

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

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

Change subject: Add "service_name" as part of ConnectionId
..

Add "service_name" as part of ConnectionId

The motivation for doing so is to allow a client to transparently use
separate connections for different services. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/proxy.cc
3 files changed, 19 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add "service name" as part of ConnectionId

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "service_name" as part of ConnectionId
..


Patch Set 2:

(3 comments)

Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h
File src/kudu/rpc/connection_id.h:

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h@56
PS1, Line 56:   // Returns a string representation of the object, not including 
the password field.
:   std::string ToString() const;
> Looks like this isn't implemented. We can consider removing this.
Done


http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc
File src/kudu/rpc/connection_id.cc:

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@52
PS1, Line 52: string ConnectionId::ToString() const {
> Need to incorporate service_name_ here too.
Done


http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@69
PS1, Line 69:   boost::hash_combine(seed, hostname_);
> We need to include the service_name_ as part of the HashCode too.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:34:20 +
Gerrit-HasComments: Yes

[kudu-CR] Add "service name" as part of ConnectionId

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11681


Change subject: Add "service_name" as part of ConnectionId
..

Add "service_name" as part of ConnectionId

The motivation for doing so is to allow a client to transparently use
separate connections for different services. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/proxy.cc
3 files changed, 16 insertions(+), 5 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] Add const attribute to RpcContext::GetInboundSidecar()

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11150


Change subject: Add const attribute to RpcContext::GetInboundSidecar()
..

Add const attribute to RpcContext::GetInboundSidecar()

Testing done: rpc-test

Change-Id: Ie1d959c40aa119389b09c9f6ac2afe7445066a38
---
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
2 files changed, 2 insertions(+), 2 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie1d959c40aa119389b09c9f6ac2afe7445066a38
Gerrit-Change-Number: 11150
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has abandoned this change. ( http://gerrit.cloudera.org:8080/8895 )

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..


Abandoned

Not needed after the fix of IMPALA-5518
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9840 )

Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..


Patch Set 2:

Many tests failed with:

Runtime error: Failed to start a single Master: 
/tmp/dist-test-task3oJsII/build/asan/bin/kudu-master: process exited on signal 6

Seems to be the same flakiness with the clock source:

F0328 19:19:56.575935  9523 master_main.cc:74] Check failed: _s.ok() Bad 
status: Service unavailable: Cannot initialize clock: Error reading clock. 
Clock considered unsynchronized
*** Check failure stack trace: ***
@ 0x7ffb3f59062d  google::LogMessage::Fail() at ??:0
@ 0x7ffb3f59264c  google::LogMessage::SendToLog() at ??:0
@ 0x7ffb3f590189  google::LogMessage::Flush() at ??:0
@ 0x7ffb3f592fdf  google::LogMessageFatal::~LogMessageFatal() at ??:0
@   0x51eb6f  kudu::master::MasterMain() at 
/home/jenkins-slave/workspace/kudu-master/2/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/asan/asan_stack.h:48
@   0x51e135  main at 
/home/jenkins-slave/workspace/kudu-master/2/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/asan/asan_interceptors.cc:42
@ 0x7ffb3c997f45  __libc_start_main at ??:0
@   0x420cba  (unknown) at ??:0
/home/jenkins-slave/workspace/kudu-master/2/src/kudu/integration-tests/ts_itest-base.cc:146:
 Failure
Failed
Bad status: Runtime error: Failed to start a single Master: 
/tmp/dist-test-task3oJsII/build/asan/bin/kudu-master: process exited on signal 6


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9840
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 28 Mar 2018 20:13:09 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9840 )

Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..


Patch Set 2:

(1 comment)

Hi Todd, good suggestion. It'd be better if this is done in another patch as we 
have multiple of them and I don't want to complicate this patch.

http://gerrit.cloudera.org:8080/#/c/9840/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9840/1//COMMIT_MSG@16
PS1, Line 16: confirmed on SLES11 that the new credential
: is being inserted by checking the 'auth time' of the ticket
: in ccache.
> nit: Could you also add that this was found on SLES11 because Impala has a
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9840
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 28 Mar 2018 19:15:59 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon,

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

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

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

Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..

KUDU-2385: Fix typo in KinitContext::DoRenewal()

On platforms without krb5_get_init_creds_opt_set_out_ccache(),
krb5_cc_store_cred() is called to insert the newly acquired
credential into the ccache. However, there was a typo in the code
which resulted in inserting the old credential into ccache.
This change fixes the typo to make sure the new credential is
inserted into ccache.

Testing done: confirmed on SLES11 that the new credential
is being inserted by checking the 'auth time' of the ticket
in ccache. Impala uses a slightly different #ifdef which
explicitly checks if krb5_get_init_creds_opt_set_out_ccache()
is defined on the platform so this code path is actually
used when running Impala on SLES11.

Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
---
M src/kudu/security/init.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9840
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9840 )

Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..


Patch Set 1:

Todd, we use this macro here: 
https://github.com/apache/impala/blob/master/be/CMakeLists.txt#L349-L350

CHECK_LIBRARY_EXISTS("krb5" krb5_get_init_creds_opt_set_out_ccache
  ${KERBEROS_LIBRARY} HAVE_KRB5_GET_INIT_CREDS_OPT_SET_OUT_CCACHE)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9840
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 28 Mar 2018 19:03:36 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9840 )

Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..


Patch Set 1:

Build failure doesn't look relevant: RaftConsensusQuorumTest.TestRequestVote

Failing for the past 1 build (Since Failed#12725 )
Took 0 ms.
Error Message
thread_collision_warner.cc:23] Thread Collision! Previous thread id: 425, 
current thread id: 770
Stacktrace

thread_collision_warner.cc:23] Thread Collision! Previous thread id: 425, 
current thread id: 770
@   0x45838e __tsan::CallUserSignalHandler() at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1819
@   0x45930b rtl_sigaction() at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1907
@ 0x7f1af83dcc37 gsignal at ??:0
@ 0x7f1af83e0028 abort at ??:0
@   0x47f098 __interceptor_abort at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1675
@ 0x7f1afaeae2b5 base::DCheckAsserter::warn() at ??:0
@ 0x7f1afaeae32e base::ThreadCollisionWarner::EnterSelf() at ??:0
@ 0x7f1b032d98e9 
base::ThreadCollisionWarner::ScopedRecursiveCheck::ScopedRecursiveCheck() at 
??:0
@ 0x7f1b032d75c2 kudu::consensus::ConsensusMetadata::current_term() at 
??:0
@ 0x7f1b0332038c kudu::consensus::RaftConsensus::CurrentTermUnlocked() 
at ??:0
@ 0x7f1b0332be31 
kudu::consensus::RaftConsensus::HandleLeaderRequestTermUnlocked() at ??:0
@ 0x7f1b0332c8f0 
kudu::consensus::RaftConsensus::CheckLeaderRequestUnlocked() at ??:0
@ 0x7f1b03328f95 kudu::consensus::RaftConsensus::UpdateReplica() at ??:0
@ 0x7f1b03328a81 kudu::consensus::RaftConsensus::Update() at ??:0
@   0x510a7c 
kudu::consensus::LocalTestPeerProxy::SendUpdateRequest() at 
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/consensus/consensus-test-util.h:?
@   0x511bb8 boost::_mfi::mf2<>::operator()() at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/tsan/include/boost/bind/mem_fn_template.hpp:280
 (discriminator 3)
@   0x511aed boost::_bi::list3<>::operator()<>() at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/tsan/include/boost/bind/bind.hpp:399
@   0x511a14 boost::_bi::bind_t<>::operator()() at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/tsan/include/boost/bind/bind.hpp:1223
@   0x5117a2 
boost::detail::function::void_function_obj_invoker0<>::invoke() at 
/home/jenkins-slave/workspace/kudu-master/1/thirdparty/installed/tsan/include/boost/function/function_template.hpp:160
@ 0x7f1afdd12c02 boost::function0<>::operator()() at ??:0
@ 0x7f1afb29c66e kudu::FunctionRunnable::Run() at ??:0
@ 0x7f1afb2980d9 kudu::ThreadPool::DispatchThread() at ??:0
@ 0x7f1afb2a2ea7 boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7f1afb2a2dfb boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7f1afb2a2d84 boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7f1afb2a2b8a 
boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7f1afdd12c02 boost::function0<>::operator()() at ??:0


 Help us localize this page Page generated: Mar 28, 2018 11:52:11 AM PDTREST 
APIJenkins ver. 2.46.3


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9840
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 28 Mar 2018 18:52:39 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2385: Fix typo in KinitContext::DoRenewal()

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9840


Change subject: KUDU-2385: Fix typo in KinitContext::DoRenewal()
..

KUDU-2385: Fix typo in KinitContext::DoRenewal()

On platforms without krb5_get_init_creds_opt_set_out_ccache(),
krb5_cc_store_cred() is called to insert the newly acquired
credential into the ccache. However, there was a typo in the code
which resulted in inserting the old credential into ccache.
This change fixes the typo to make sure the new credential is
inserted into ccache.

Testing done: confirmed on SLES11 that the new credential
is being inserted by checking the 'auth time' of the ticket
in ccache.

Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
---
M src/kudu/security/init.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3a22b8d41d15eb1982a3fd5b96575e28edaad31c
Gerrit-Change-Number: 9840
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2374: Add RpcContext::GetTimeReceived()

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9796


Change subject: KUDU-2374: Add RpcContext::GetTimeReceived()
..

KUDU-2374: Add RpcContext::GetTimeReceived()

This change adds RpcContext::GetTimeReceived() which returns
the time at which the inbound call associated with the RpcContext
was received. It's helpful to make this accessible to the RPC
handlers for its own book-keeping purpose (e.g. reporting the
average dispatch latency as part of query profile in Impala).

Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
3 files changed, 10 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6b39c7f2ea856eccfdab8c1bb1433829e979ae13
Gerrit-Change-Number: 9796
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9601 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 8: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG@20
PS8, Line 20: rpc_max_message_size
Seems clearer to say FLAGS_rpc_max_message_size


http://gerrit.cloudera.org:8080/#/c/9601/8//COMMIT_MSG@21
PS8, Line 21: any message with sidecars this large
regardless of whether there is any sidecar, "any message this large" is 
rejected, right ? Of course, it's unlikely for such large message to not have 
sidecar.


http://gerrit.cloudera.org:8080/#/c/9601/8/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/9601/8/src/kudu/rpc/rpc-test.cc@709
PS8, Line 709:   string max_string;
 :   max_string.resize(TransferLimits::kMaxTotalSidecarBytes, 'a');
string max_string(TransferLimits::kMaxTotalSidecarBytes, 'a');



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 21 Mar 2018 20:40:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9601 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:

http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.h@253
PS3, Line 253: int64_t
I wonder if this should also be int32_t to match the equivalent in OutboundCall 
?


http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:

http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/inbound_call.cc@225
PS3, Line 225:   outbound_sidecars_total_bytes_ += sidecar_bytes;
If you end up converting outbound_sidecars_total_bytes_ to int32_t as suggested 
elsewhere, you may wanna DCHECK_GE(outbound_sidecrs_total_bytes_, 0) here.


http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/outbound_call.h@276
PS3, Line 276:   // Total size in bytes of all sidecars in 'sidecars_'. Set in 
SetRequestPayload().
Add a comment that this should not exceed TransferLimits::kMaxTotalSidecarBytes


http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

http://gerrit.cloudera.org:8080/#/c/9601/3/src/kudu/rpc/rpc_controller.h@275
PS3, Line 275: int64_t
Again, we seem a bit inconsistent on whether this should be 32-bit or 64-bit.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Mar 2018 22:44:58 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9601 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161:  // SerializeMessage() takes the additional_size as an int32_t. 
Protect against
 :   // any overflow.
 :   CHECK_LE(sidecar_byte_size_, 
std::numeric_limits::max());
> In the abstract, using int32_t can make the compiler detect problems. Compi
If additional_size is less than INT_MAX in practice, is it possible to change 
sidecar_byte_size_ to int32_t instead ? In which case, there is no mismatch.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:22:31 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9601 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161:  // SerializeMessage() takes the additional_size as an int32_t. 
Protect against
 :   // any overflow.
 :   CHECK_LE(sidecar_byte_size_, 
std::numeric_limits::max());
> Maybe these should be DCHECKs. We know that they can't be triggered, becaus
I still find the type mismatch a bit confusing.

Given the computation of total size in SerializeMessage() is using int64_t and 
sidecar_byte_size_ is also int64_t, I don't see why we just standardize on just 
switching the type of additional_size to int64_t and skip the cast inside 
SerialzieMessage() ? What's the benefit of keeping it as int32_t given 64-bit 
machines have 64-bit registers anyway.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 14 Mar 2018 20:22:00 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9601 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/util/net/socket.cc@451
PS2, Line 451: int res
Just wondering if there is any guarantee the return value will not overflow an 
int ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 14 Mar 2018 19:05:01 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2305: Limit sidecars to INT MAX and fortify socket code

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9601 )

Change subject: KUDU-2305: Limit sidecars to INT_MAX and fortify socket code
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/outbound_call.cc@161
PS2, Line 161:  // SerializeMessage() takes the additional_size as an int32_t. 
Protect against
 :   // any overflow.
 :   CHECK_LE(sidecar_byte_size_, 
std::numeric_limits::max());
Why not just replace additional_size with uint32_t and avoid this CHECK() 
altogether ?


http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/serialization.cc
File src/kudu/rpc/serialization.cc:

http://gerrit.cloudera.org:8080/#/c/9601/2/src/kudu/rpc/serialization.cc@57
PS2, Line 57: int additional_size
Why is this not uint32_t ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d041e214b15d9c22b810588643798e2b3bc5c24
Gerrit-Change-Number: 9601
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 14 Mar 2018 18:56:42 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9587 )

Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with 
SSL_write()
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9587/2/src/kudu/rpc/rpc-test.cc@1225
PS2, Line 1225: // This serves as a helper function for 
TestCancellationMultiThreads() to exercise
  : // cancellation when there are concurrent RPCs.
> nit: Better to not explain what a user of this function would do, since it'
Doesn't seem too bad to as this provides context on how this function fits in 
with the rest of the test.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
Gerrit-Change-Number: 9587
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 13 Mar 2018 06:43:50 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9587 )

Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with 
SSL_write()
..


Patch Set 2:

Hit with flaky tests again in TSAN build:

[  FAILED  ] AllTypesItest/3.TestAllKeyTypes, where TypeParam = 
kudu::client::IntKeysTestSetup >
[  FAILED  ] AllTypesItest/7.TestAllKeyTypes, where TypeParam = 
kudu::client::IntKeysTestSetup 
>

Both failed with the following backtraces:

F0313 01:15:33.904728 14346 master_main.cc:74] Check failed: _s.ok() Bad 
status: Service unavailable: Cannot initialize clock: Error reading clock. 
Clock considered unsynchronized
*** Check failure stack trace: ***
@ 0x7f9641ff03b9  google::LogMessage::Flush() at ??:0
@ 0x7f9641ff446e  google::LogMessageFatal::~LogMessageFatal() at ??:0
@   0x4b5391  kudu::master::MasterMain() at 
/home/jenkins-slave/workspace/kudu-master/3/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:6199
@   0x4b4ddf  main at 
/home/jenkins-slave/workspace/kudu-master/3/thirdparty/src/llvm-4.0.0.src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:6189
@ 0x7f963f379f45  __libc_start_main at ??:0
@   0x41e092  (unknown) at ??:0
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/integration-tests/all_types-itest.cc:518:
 Failure


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
Gerrit-Change-Number: 9587
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 13 Mar 2018 01:47:34 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9587 )

Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with 
SSL_write()
..


Patch Set 1:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test-base.h@441
PS1, Line 441:   FLAGS_rpc_encrypt_loopback_connections = true;
> ah, does this mean we were not successfully testing encrypted RPCs in the p
I believe so. I confirmed that the TlsSocket path is now being exercised by 
manually adding a CHECK() in that TlsSocket::Write().


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@740
PS1, Line 740:// Linux, SSL enabled
> hmm, is this fix related to the patch in question? I'm surprised we haven't
Yes, as we weren't using TlsSocket before so we weren't triggering this error 
message.


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1223
PS1, Line 1223: // Helper function for TestCancellationMultiThreads.
> better to document briefly what it does, rather than what it is.
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1224
PS1, Line 1224: SendRpcAndCancel
> this should probably be named SendAndCancelRpcs or something, since it send
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1251
PS1, Line 1251: ASSERT_TRUE
> have to use CHECK here since this isn't the main thread and gtest isn't thr
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1256
PS1, Line 1256: #define BUF_SIZE (16 * 1024 * 1024)
> can you use a const int defined below within the function instead?  We pref
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1258
PS1, Line 1258: Regression
> did this test successfully repro the bug w/o the fix?
Unfortunately, no even with multiple tries in different build types. That said, 
I think this is still a good test to include. Any suggestion to make this test 
more effective in triggering the bug is welcomed.

I confirmed the fix with Impala stress test running on a 130 node cluster which 
hit this bug quite consistently without the fix. Updated the commit message 
with this piece of information.


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1274
PS1, Line 1274:   unique_ptr buf(new uint8_t[BUF_SIZE]);
  :   memset(buf.get(), 'a', BUF_SIZE);
  :   Slice slice(buf.get(), BUF_SIZE);
> you could probably get away with just:
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h@186
PS1, Line 186: True if we have tried sending anything in 'payload_slices_'
> Actually isn't it more like "True if SendBuffer() is called at least once."
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h@187
PS1, Line 187:   // no bytes were sent successfully. This is needed as 
SSL_write() is stateful.
> nit: Add a reference to this JIRA, else it's hard to understand why we adde
Done


http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.cc@190
PS1, Line 190:   started_ = true;
> Do we know of any cases where setting started_ = true this early can backfi
As far as I can tell, this should be fine. Both SendBuffer() and 
OutboundTransfer::TransferStarted() are called from Connection::WriteHandler() 
only. So, if we get to the point of SendBuffer(), we should have taken the path 
of if (!transfer->TransferStarted()) once.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
Gerrit-Change-Number: 9587
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 13 Mar 2018 00:54:24 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon,

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

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

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

Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with 
SSL_write()
..

KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL_write()

Previously, OutboundTransfer::TransferStarted() returns true iff
non-zero bytes have been successfully sent via Writev(). As it turns
out, this doesn't work well with SSL_write(). When SSL_write() returns -1
with errno EAGAIN or ETRYAGAIN, we need to retry the call with exactly
the same buffer pointer next time even if 0 bytes have been written.

The following sequence becomes problematic with the previous implementation
of OutboundTransfer::TransferStarted():

- WriteHandler() calls SendBuffer() on an OutboundTransfer.
- SendBuffer() calls TlsSocket::Writev() which hits the EAGAIN error above.
  Since 0 bytes were written, cur_slice_idx_ and cur_offset_in_slice_ remain 0
  and OutboundTransfer::TransferStarted() still returns false.
- OutboundTransfer is cancelled or timed out. car->call is set to NULL.
- WirteHandler() is called again and as it notices that the OutboundTransfer
  hasn't really started yet and "car->call" is NULL due to cancellation, it
  removes it from the outbound transfer queue and moves on to the next entry
  in the queue.
- WriteHandler() calls SendBuffer() with the next entry in the queue and
  eventually calls SSL_write() with a different buffer than expected by
  SSL_write(), leading to "SSL3_WRITE_PENDING:bad write retry" error.

This change fixes the problem above by adding a boolean flag 'started_'
which is set to true if OutboundTransfer::SendBuffer() has been called
at least once. Also added some tests to exercise cancellation paths with
multiple concurrent RPCs.

Confirmed the problem above is fixed by running stress test in a 130 node
cluster with Impala. The problem happened consistently without the fix.

Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
4 files changed, 84 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
Gerrit-Change-Number: 9587
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9587 )

Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with 
SSL_write()
..


Patch Set 1:

the test failure seems to be infrastructure issue ?

tablet-decoder-eval-test.0  dist-test-slave-dist-test-slave-p2pc-2  
Nonefailed to download task files: WARNING 100 isolateserver(1484): 
Adding unknown file 9a0053f2a34


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
Gerrit-Change-Number: 9587
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 12 Mar 2018 21:23:21 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL write()

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9587


Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with 
SSL_write()
..

KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL_write()

Previously, OutboundTransfer::TransferStarted() returns true iff
non-zero bytes have been successfully sent via Writev(). As it turns
out, this doesn't work well with SSL_write(). When SSL_write() returns -1
with errno EAGAIN or ETRYAGAIN, we need to retry the call with exactly
the same buffer pointer next time even if 0 bytes have been written.

The following sequence becomes problematic with the previous implementation
of OutboundTransfer::TransferStarted():

- WriteHandler() calls SendBuffer() on an OutboundTransfer.
- SendBuffer() calls TlsSocket::Writev() which hits the EAGAIN error above.
  Since 0 bytes were written, cur_slice_idx_ and cur_offset_in_slice_ remain 0
  and OutboundTransfer::TransferStarted() still returns false.
- OutboundTransfer is cancelled or timed out. car->call is set to NULL.
- WirteHandler() is called again and as it notices that the OutboundTransfer
  hasn't really started yet and "car->call" is NULL due to cancellation, it
  removes it from the outbound transfer queue and moves on to the next entry
  in the queue.
- WriteHandler() calls SendBuffer() with the next entry in the queue and
  eventually calls SSL_write() with a different buffer than expected by
  SSL_write(), leading to "SSL3_WRITE_PENDING:bad write retry" error.

This change fixes the problem above by adding a boolean flag 'started_'
which is set to true if OutboundTransfer::SendBuffer() has been called
at least once. Also added some tests to exercise cancellation paths with
multiple concurrent RPCs.

Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
---
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
4 files changed, 83 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683
Gerrit-Change-Number: 9587
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2305: Fix local variable usage to handle 2GB messages

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9355 )

Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages
..


Patch Set 3:

Ping. Impala also needs this patch once this is merged on Kudu side.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c
Gerrit-Change-Number: 9355
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 22 Feb 2018 04:34:02 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2305: Fix local variable usage to handle 2GB messages

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9355 )

Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/serialization.cc
File src/kudu/rpc/serialization.cc:

http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/serialization.cc@61
PS3, Line 61: static_cast(additional_size)
Won't this be automatically promoted to int64_t ? Is this second cast needed ? 
Same below.


http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/transfer.cc@81
PS3, Line 81: kMsgLengthPrefixLength
Does it make sense to to DCHECK_LE(kMsgLengthPrefixLength, INT_MAX) or in 
theory, this may overflow rem here too.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c
Gerrit-Change-Number: 9355
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 20 Feb 2018 03:17:03 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@82
PS3, Line 82:  rolling_kbps_.set_capacity(1000);
move this to initialization list rolling_kbps_(1000) ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:44:40 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
 : Stopwatch send_timer;
 : send_timer.start();
 : Status status = transfer->SendBuffer(*socket_);
 : send_timer.stop();
Not sure how accurate this will be fore inferring throughput given this is 
non-blocking.

Also, will it be sufficient to approximate the Kbps based on the aggregate of 
all loop iterations instead of doing it per loop iteration ?


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/transfer.cc@245
PS3, Line 245: if (TransferFinished()) return 0;
nit: Not sure if Kudu coding standard allows this kind of one liner.


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/transfer.cc@246
PS3, Line 246:   int32_t rem_in_cur_slice = 
payload_slices_[cur_slice_idx_].size() - cur_offset_in_slice_;
 :   int32_t total_remaining = rem_in_cur_slice;
Why not just write

int32_t total_remaining = payload_slices_[cur_slice_idx_].size() - 
cur_offset_in_slice_;



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:32:27 +
Gerrit-HasComments: Yes

[kudu-CR] rpc: micro-optimize delayed task handling

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9048 )

Change subject: rpc: micro-optimize delayed task handling
..


Patch Set 3: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:30:12 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8895 )

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..


Patch Set 4:

Thanks for the review. We need to re-evaluate the need for it after IMPALA-5518 
is fixed. It's unclear at this point whether this is manifesting as a 
side-effect of that bug.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 25 Jan 2018 01:05:23 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9117 )

Change subject: KUDU-2270: Add a flag to control logging in 
RpczStore::LogTrace()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9117/3/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/9117/3/src/kudu/rpc/rpc-test.cc@74
PS3, Line 74: DECLARE_int32(rpc_duration_too_long_ms);
Sorry, forgot to remove this line here. I can push another patch.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9117
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 24 Jan 2018 20:21:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9117 )

Change subject: KUDU-2270: Add a flag to control logging in 
RpczStore::LogTrace()
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9117/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/9117/1/src/kudu/rpc/rpc-test.cc@592
PS1, Line 592: TEST_P(TestRpc, TestRpcSidecar) {
> hrm, I appreciate the effort of adding a test, but since the test doesn't a
Removed. Yes, there doesn't seem to be a way to confirm whether something is 
logged (or not) from the test. I manually inspected the output of the test to 
confirm the flag is effective.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9117
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:06:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

[Michael Ho (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2270: Add a flag to control logging in 
RpczStore::LogTrace()
..

KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

This change adds a new flag FLAGS_rpc_duration_too_long_ms
which controls the duration above which a RPC is considered
too long and is logged at INFO level in the log. Previously,
this threshold is hardcoded to 1000ms which may be too short
for a busy Impalad demon, leading to massive log spew.

Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpcz_store.cc
2 files changed, 9 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9117
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: micro-optimize delayed task handling

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9048 )

Change subject: rpc: micro-optimize delayed task handling
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9048/3/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:

http://gerrit.cloudera.org:8080/#/c/9048/3/src/kudu/rpc/periodic-test.cc@270
PS3, Line 270: SCOPED_CLEANUP({ messenger->Shutdown(); });
Just for my own education: any reason why we use SCOPED_CLEANUP() here instead 
of calling messenger->Shutdown() at the end of the function ?

I don't see any early return in this function.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 24 Jan 2018 08:50:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9117


Change subject: KUDU-2270: Add a flag to control logging in 
RpczStore::LogTrace()
..

KUDU-2270: Add a flag to control logging in RpczStore::LogTrace()

This change adds a new flag FLAGS_rpc_too_long_duration_ms
which controls the duration above which a RPC is considered
too long and is logged at INFO level in the log. Previously,
this threshold is hardcoded to 1000ms which may be too short
for a busy Impalad demon, leading to massive log spew.

Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpcz_store.cc
2 files changed, 41 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie587ee602e83bb65d74f7ee622a9bc47897f2574
Gerrit-Change-Number: 9117
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] rpc: avoid an extra copy of shared ptr for OutboundCall

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9047 )

Change subject: rpc: avoid an extra copy of shared_ptr for OutboundCall
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89356bd9c59e612e64ffc4e1ad4a6bfda44512aa
Gerrit-Change-Number: 9047
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Thu, 18 Jan 2018 01:25:09 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Sailesh Mukil, Todd Lipcon, Mostafa Mokhtar, 

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

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

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

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..

KUDU-1865: Avoid some heap allocations in RPC paths

As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.

This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object

Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
11 files changed, 100 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/8895/4
--
To view, visit http://gerrit.cloudera.org:8080/8895
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Sailesh Mukil, Todd Lipcon, Mostafa Mokhtar, 

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

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

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

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..

KUDU-1865: Avoid some heap allocations in RPC paths

As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.

This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object

Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
11 files changed, 100 insertions(+), 85 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8895 )

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@21
PS2, Line 21: Introduce a new interface RpcContext::RespondSuccess(const 
Message& message)
:   which allows callers to pass the response PB directly as 
argument instead
:   of using the preallocated response PB owned by RpcContext. This 
allows callers
:   to allocate the response PB on the stack, thus avoiding the 
need to use the heap.
> Yes, I am not too keen on having two APIs either. In fact, I wonder if ther
It'd also be nice if there is an object pool which can recycle the request and 
response buffer but it would have to be a per-thread pool or we may run into 
the lock contention when multiple service threads try to allocate from it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 21 Dec 2017 06:43:57 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8895 )

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@16
PS2, Line 16: central cache list in TCMalloc and causes spin lock contention.
> would be good to accompany this with some benchmark results
I can look into rpc-bench. The perf infrastructure I have been using is down 
recently so I cannot run the typical TPC-H and TPC-DS benchmark at this point.

An anecdotal data point is a stress test which used to take 86 mins went down 
to about 57 mins but it's not quite scientific measurement.


http://gerrit.cloudera.org:8080/#/c/8895/2//COMMIT_MSG@21
PS2, Line 21: Introduce a new interface RpcContext::RespondSuccess(const 
Message& message)
:   which allows callers to pass the response PB directly as 
argument instead
:   of using the preallocated response PB owned by RpcContext. This 
allows callers
:   to allocate the response PB on the stack, thus avoiding the 
need to use the heap.
> I'm not so keen on this API change here. It seems a bit messy to have two d
Yes, I am not too keen on having two APIs either. In fact, I wonder if there is 
a reason why all existing calls to RespondSuccess() cannot switch over to using 
the new interface and deprecate the old interface. The few RPCs I looked at 
seem to be easy to convert but there may be tricky ones which I didn't look at. 
This seems to make sense to me and we can get rid of the allocation for the 
response buffer in GeneratedServiceIf::Handle().

I haven't used 'Arena' before so would you mind explaining how it would help 
here. Does it allocate a large enough chunk to satisfy the allocations for 
request and other items so the malloc() call will be amortized ? So, in other 
words, heap allocations are still needed.

I will split the interface change out as a separate change.


http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/connection.cc@430
PS2, Line 430:   scoped_refptr conn_;
> is this required for correctness in this patch?
Yes. I ran into a case in rpc-test in which the dtor of InboundCall() 
recursively calls into the dtor of 'conn_' in which the 'inbound_call_pool_' 
resides.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 21 Dec 2017 06:32:50 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8895 )

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/service_if.h
File src/kudu/rpc/service_if.h:

http://gerrit.cloudera.org:8080/#/c/8895/2/src/kudu/rpc/service_if.h@118
PS2, Line 118:   // Allocate a response PB for the given method. The memory 
returned is owned by the RpcContext.
 :   // May be overridden by derived class.
 :   virtual google::protobuf::Message* AllocResponsePB(const 
RpcMethodInfo* method_info);
Not entirely sure this is the most preferable approach. Open to suggestions.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 21 Dec 2017 00:18:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8895 )

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/result_tracker.cc@275
PS1, Line 275:
ErrorStatusPB_RpcErrorCodePB err,
> warning: parameter 'err' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h@306
PS1, Line 306: GetTokenResponsePB* resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h@316
PS1, Line 316:   void TestArgumentsInDiffPackage(const ReqDiffPackagePB *req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc-test-base.h@317
PS1, Line 317:   RespDiffPackagePB *resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc_context.h
File src/kudu/rpc/rpc_context.h:

http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/rpc_context.h@219
PS1, Line 219:   RpcContext(InboundCall *call);
> warning: single-argument constructors must be marked explicit to avoid unin
Done


http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/service_if.h
File src/kudu/rpc/service_if.h:

http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/service_if.h@113
PS1, Line 113:   void Handle(InboundCall* incoming) override;
> warning: function 'kudu::rpc::GeneratedServiceIf::Handle' has a definition
Done


http://gerrit.cloudera.org:8080/#/c/8895/1/src/kudu/rpc/service_if.h@120
PS1, Line 120:   virtual google::protobuf::Message* AllocResponsePB(const 
RpcMethodInfo* method);
> warning: function 'kudu::rpc::GeneratedServiceIf::AllocResponsePB' has a de
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 20 Dec 2017 22:35:49 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

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

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

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

Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..

KUDU-1865: Avoid some heap allocations in RPC paths

As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.

This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object
- Introduce a new interface RpcContext::RespondSuccess(const Message& message)
  which allows callers to pass the response PB directly as argument instead
  of using the preallocated response PB owned by RpcContext. This allows callers
  to allocate the response PB on the stack, thus avoiding the need to use the 
heap.
- To complement the stack allocation of Response PB, we wrap the code which 
allocates
  the Response PB in a new function GeneratedServiceIf::AllocResponsePB() which 
allows
  derived classes to override it. If a service decides that all response PB 
will be
  allocated on the stack, the overridden function can always return a nullptr.

Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
13 files changed, 165 insertions(+), 85 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1865: Avoid some heap allocations in RPC paths

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8895


Change subject: KUDU-1865: Avoid some heap allocations in RPC paths
..

KUDU-1865: Avoid some heap allocations in RPC paths

As shown in KUDU-1865 and IMPALA-5528, KRPC tends to put a lot
of stress on the thread caches of TCMalloc. In particular, the
RPC code allocates quite a number of small objects (e.g.
InboundCall, RpcContext, Request PB, Response PB etc) per RPC.
Under high rate of RPC, the free list of the thread caches will
get exhausted during allocations and then overflow once all the
small objects are freed. This leads to frequent trips to the
central cache list in TCMalloc and causes spin lock contention.

This change relieves some of the pressure in the thread cache by:
- Recycle InboundCall objects with an ObjectPool
- Embed RpcContext into the InboundCall instead of having a separate object
- Introduce a new interface RpcContext::RespondSuccess(const Message& message)
  which allows callers to pass the response PB directly as argument instead
  of using the preallocated response PB owned by RpcContext. This allows callers
  to allocate the response PB on the stack, thus avoiding the need to use the 
heap.
- To complement the stack allocation of Response PB, we wrap the code which 
allocates
  the Response PB in a new function GeneratedServiceIf::AllocResponsePB() which 
allows
  derived classes to override it. If a service decides that all response PB 
will be
  allocated on the stack, the overridden function can always return a nullptr.

Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
13 files changed, 161 insertions(+), 81 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I407b4782c9f3cd39ad3c6e0d21fd9542be34b118
Gerrit-Change-Number: 8895
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to any negative value. This avoids the unnecessary
overhead of scanning for idle server connections and alleviates
the user from having to pick a random large number to make sure
the connection is always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/server/server_base.cc
6 files changed, 75 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/8831/8
--
To view, visit http://gerrit.cloudera.org:8080/8831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to -1 or any negative value. This avoids the unnecessary
overhead of scanning for idle server connections and alleviates
the user from having to pick a random large number to make sure
the connection is always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/server/server_base.cc
6 files changed, 75 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/8831/7
-- 
To view, visit http://gerrit.cloudera.org:8080/8831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to -1 or any negative value. This avoids the unnecessary
overhead of scanning for idle server connections and alleviates
the user from having to pick a random large number to make sure
the connection is always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 76 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/8831/6
--
To view, visit http://gerrit.cloudera.org:8080/8831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8831 )

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8831/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8831/5//COMMIT_MSG@20
PS5, Line 20: set to -1 or any negative value. This avoids the unnecessary
> I think -1 would make more sense as a special value. '0' sounds like 'keepa
Makes sense. In fact, setting it to a negative value won't be too meaningful so 
idle connection detection is only run when keep_alive_time_ms >= 0. Please let 
me know if you think otherwise.


http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc@402
PS5, Line 402:   // Set up server.
> agreed with tidybot here
Some earlier version without {} gets flagged by LINT:

/home/jenkins-slave/workspace/kudu-master/0/src/kudu/rpc/rpc-test.cc:400:  
Empty loop bodies should use {} or continue  [whitespace/empty_loop_body] [5]

I can switch to using an empty body. In fact, this is gone in PS6.


http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc@407
PS5, Line 407: nt.
> 'AlwaysKeepalive" is more accurate than 'NoTimeout' since we use the term '
Done


http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc@435
PS5, Line 435: 
 : // Test that outbound connections to the same server are reopen 
upon every RPC
 : // call when the 'rpc_reopen_outbound_connections' flag is set.
 : TEST_P(TestRpc, TestReopenOutboundConnections) {
 :   // Set the flag to enable special mode: close and reopen 
already established
> I'm not entirely following this test. You're trying to trigger KUDU-279, bu
The simple test you suggested is essentially line 445 - line 463 below. What I 
am concerned about the simple test is that we cannot really tell if the 
connection is still alive because the thread didn't sleep for long enough.

I was also a bit uncertain whether to keep the more complicated test. On one 
hand, it serves as an example which inspired this change to begin with and 
verifies that setting keepalive_time_ms_ = 0 will keep the connection alive and 
"solves" the problem. On the other hand, this seems a bit hard to follow and it 
may break once KUDU-279 is fixed.

I simplify this call to a synchronous RPC in the new PS and keeps the simple 
test around.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 19 Dec 2017 08:04:00 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

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

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

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

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to 0. This avoids the unnecessary overhead of scanning
for idle server connections and alleviates the user from having
to pick a random large number to make sure the connection is
always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 105 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/8831/5
--
To view, visit http://gerrit.cloudera.org:8080/8831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

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

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

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

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to 0. This avoids the unnecessary overhead of scanning
for idle server connections and alleviates the user from having
to pick a random large number to make sure the connection is
always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 104 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/8831/4
--
To view, visit http://gerrit.cloudera.org:8080/8831
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to 0. This avoids the unnecessary overhead of scanning
for idle server connections and alleviates the user from having
to pick a random large number to make sure the connection is
always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 104 insertions(+), 28 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8831


Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
..

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to 0. This avoids the unnecessary overhead of scanning
for idle server connections and alleviates the user from having
to pick a random large number to make sure the connection is
always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 102 insertions(+), 28 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2228: Make Messenger options configurable

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8789 )

Change subject: KUDU-2228: Make Messenger options configurable
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.h@176
PS3, Line 176:   std::string keytab_file_ = "";
Is there any reason why FLAGS_rpc_tls_ciphers and FLAGS_rpc_tls_min_protocol 
and possibly other flags used in  TlsContext::Init() are not converted to 
options configurable in messenger builder ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 08:53:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2228: Make Messenger options configurable

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8789 )

Change subject: KUDU-2228: Make Messenger options configurable
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:

http://gerrit.cloudera.org:8080/#/c/8789/3/src/kudu/rpc/messenger.cc@124
PS3, Line 124: std::string sasl_proto_name
Seems more consistent to copy here too like other functions unless there is a 
reason not to.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8789
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 08:42:12 +
Gerrit-HasComments: Yes

[kudu-CR] [security] Make the kerberos principal configurable for Kudu servers

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8700 )

Change subject: [security] Make the kerberos principal configurable for Kudu 
servers
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8700/1/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/8700/1/src/kudu/security/init.cc@382
PS1, Line 382: raw_principal
in_principal seems like a better name.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695
Gerrit-Change-Number: 8700
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Fri, 01 Dec 2017 07:08:58 +
Gerrit-HasComments: Yes

[kudu-CR] Reduce log spew from rpcz store.cc

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8273


Change subject: Reduce log spew from rpcz_store.cc
..

Reduce log spew from rpcz_store.cc

The sampled RPC call statement has been filing the log. Let's dial down its log 
level.
This particularly impacts Impala as its default log level is 1.

If it turns out that this log statement is useful for Kudu deployment, we can
consider doing this change in Impala only.

Testing done:
  ctest -R rpc-test;
  Built debug and release builds;

Change-Id: I8efe68be0ae7f9ab05937d5c81fb53a709a881f1
---
M src/kudu/rpc/rpcz_store.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8efe68be0ae7f9ab05937d5c81fb53a709a881f1
Gerrit-Change-Number: 8273
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 5:

The verification failure may be infra related. Not sure if I have the 
permission to request a re-run.

13:52:46 F0817 20:52:08.278630 31810 master_main.cc:68] Check failed: _s.ok() 
Bad status: Network error: error binding socket to 0.0.0.0:40300: Address 
already in use (error 98)
13:52:46 *** Check failure stack trace: ***

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..

KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Previously, when an outbound call times out after transmission
of the request has begun but not finished, the RPC layer will
continue to send the entire payload (including any sidecars) to
the remote destination until the promised number of bytes specified
in the header are sent. This is problematic for the users of the
RPC layer as there is no well defined point in which the sidecars
are no longer referenced by the RPC layer. The only model in which
this would work is for the caller to either transfer or share ownership
of the sidecars with the RPC layer. This has caused some life cycle
issues with row batches in Impala before (e.g. IMPALA-5093).

This change fixes the problem above by modifying the RPC protocol
to allow a mid-transmission RPC call to be cancelled. Specifically,
a new footer is added to all outbound call requests. It contains
a flag, when true, indicates the outbound call was cancelled after
it has started but before it finished. The server will inspect this
flag and ignore inbound calls with this flag set. This footer enables
the caller to relinquish references to the sidecars early when an
outbound call is cancelled or timed-out. Once the call is cancelled
or timed-out, the RPC layer will send the remainder of the bytes for
the request with some dummy bytes. Since the server always ignores
cancelled call requests, it's okay to send random bytes.

To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS
is introduced in this change. During connection negotiation, the
client will always include this flag in its feature set. A server which
supports parsing footer will include REQUEST_FOOTERS in its feature set
if it sees the client also supports it. A client will only send the
footer if the remote server has the RPC feature flag REQUEST_FOOTERS.

Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
---
M docs/design-docs/rpc.md
M src/kudu/rpc/blocking_ops.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/constants.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/serialization.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
19 files changed, 532 insertions(+), 159 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 254:   // sidecars attached to the call as it may result in 
use-after-free of the sidecars.
> per comment in header, I think it's worth explaining why this is reasonable
Added some explanation in the header. Refer to it here.


PS3, Line 273: outbound transfer for this call so references to the sidecars
 :   // can be relinquished.
> I think this is no longer necessary to be added since the explanation of th
Done


http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS3, Line 304: e.g. older version of Kudu server), it's fatal for the
> it's not clear what behavior this is implying. If it will FATAL, I think we
Done


http://gerrit.cloudera.org:8080/#/c/7599/3/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

Line 223: return sidecar_byte_size_ > 0;
> this isn't quite accurate to what the description says. It is possible to h
Right. Comments updated. Renamed function to HasNonEmptySidecars().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..

KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Previously, when an outbound call times out after transmission
of the request has begun but not finished, the RPC layer will
continue to send the entire payload (including any sidecars) to
the remote destination until the promised number of bytes specified
in the header are sent. This is problematic for the users of the
RPC layer as there is no well defined point in which the sidecars
are no longer referenced by the RPC layer. The only model in which
this would work is for the caller to either transfer or share ownership
of the sidecars with the RPC layer. This has caused some life cycle
issues with row batches in Impala before (e.g. IMPALA-5093).

This change fixes the problem above by modifying the RPC protocol
to allow a mid-transmission RPC call to be cancelled. Specifically,
a new footer is added to all outbound call requests. It contains
a flag, when true, indicates the outbound call was cancelled after
it has started but before it finished. The server will inspect this
flag and ignore inbound calls with this flag set. This footer enables
the caller to relinquish references to the sidecars early when an
outbound call is cancelled or timed-out. Once the call is cancelled
or timed-out, the RPC layer will send the remainder of the bytes for
the request with some dummy bytes. Since the server always ignores
cancelled call requests, it's okay to send random bytes.

To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS
is introduced in this change. During connection negotiation, the
client will always include this flag in its feature set. A server which
supports parsing footer will include REQUEST_FOOTERS in its feature set
if it sees the client also supports it. A client will only send the
footer if the remote server has the RPC feature flag REQUEST_FOOTERS.

Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
---
M docs/design-docs/rpc.md
M src/kudu/rpc/blocking_ops.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/constants.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/serialization.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
19 files changed, 532 insertions(+), 160 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/7599/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7599
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..

KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Previously, when an outbound call times out after transmission
of the request has begun but not finished, the RPC layer will
continue to send the entire payload (including any sidecars) to
the remote destination until the promised number of bytes specified
in the header are sent. This is problematic for the users of the
RPC layer as there is no well defined point in which the sidecars
are no longer referenced by the RPC layer. The only model in which
this would work is for the caller to either transfer or share ownership
of the sidecars with the RPC layer. This has caused some life cycle
issues with row batches in Impala before (e.g. IMPALA-5093).

This change fixes the problem above by modifying the RPC protocol
to allow a mid-transmission RPC call to be cancelled. Specifically,
a new footer is added to all outbound call requests. It contains
a flag, when true, indicates the outbound call was cancelled after
it has started but before it finished. The server will inspect this
flag and ignore inbound calls with this flag set. This footer enables
the caller to relinquish references to the sidecars early when an
outbound call is cancelled or timed-out. Once the call is cancelled
or timed-out, the RPC layer will send the remainder of the bytes for
the request with some dummy bytes. Since the server always ignores
cancelled call requests, it's okay to send random bytes.

To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS
is introduced in this change. During connection negotiation, the
client will always include this flag in its feature set. A server which
supports parsing footer will include REQUEST_FOOTERS in its feature set
if it sees the client also supports it. A client will only send the
footer if the remote server has the RPC feature flag REQUEST_FOOTERS.

Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
---
M docs/design-docs/rpc.md
M src/kudu/rpc/blocking_ops.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/constants.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/serialization.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
19 files changed, 526 insertions(+), 160 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251: } else {
> yea I think that is actually reasonable. I'd do CHECK though, since this is
Done


PS2, Line 268: 
 :   // The timeout timer is
> per above, this is still best-effort in the case that you are talking to a 
Comments updated.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 199: etes.
> Comment rephrased.
Actually, comment removed.


PS2, Line 201:  the negotiation thread 
> ah, I see... negotiation_complete_ is set by the reactor thread itself wher
Yes, added a small note about this in the code.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

PS2, Line 352:   // behavior is a lot more efficient if memory is freed from 
the same thread
 :   // which allocated it -- this le
> looking at the implementation of faststring::release() I think these calls 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> Thanks for summarizing.
I just did a grep in the Kudu code. There doesn't seem to be any use of 
RpcController::AddOutboundSidecar(). I suppose if Kudu starts using client 
sidecar for some new functionality, it needs to update the Kudu server code to 
make use of the sidecars too so mixed version doesn't really apply in this case.

May be it's safe to just add a DCHECK to verify that the sidecars' bytes are 
zero if remote doesn't support footers or will it cause any confusion ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> hm... to summarize the possibilities here:
Thanks for summarizing.

#3 seems attractive but I am a bit worried about the arbitrarily large sidecars 
which we have to buffer. It may end up pushing Impalad over the process memory 
limit and getting it OOM killed.

For all practical purposes in Impala, we envision the first version of Impala 
using KRPC will have both server and client supporting footers. #2 is not ideal 
for mixed cluster usage with client sidecars but arguably it's not a regression 
either. I don't know how common the usage of client sidecars in Kudu today is. 
If Impala is the only use case, may be it's safer to stick with #2 for now.

Yet another idea is to fabricate certain byte sequence which will always fail 
to deserialize in the remote server but I don't think it will work once the 
request PB has been sent so I won't bet on it.

Please let me know what you think.


PS2, Line 703:if (!transfer->TransferStarted()) {
 :   if (transfer->is_for_outbound_call()) {
 : if (!StartCallTransfer(transfer)) {
> these can all be collapsed into a single boolean expression now, right?
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 199: takes
> typo: take
Comment rephrased.


PS2, Line 199: Callers should takes this into account.
> not quite sure what this sentence is indicating. Don't we assume that calle
Rephrased the comment. It intends to point out that this function will return 
false before negotiation completes so callers (in particular reactor threads) 
need to handle it properly by doing the "right thing" (whatever that means for 
the caller).


PS2, Line 284: sending it for
 :   // the first time
> I think better to say "beginning to send it" or "beginning transmission" or
Done


Line 288:   // If the checks pass, the outbound call will be set to 'SENDING' 
state.
> if the checks don't pass, is this responsible for 'finishing' the call? eg 
Comments added.


PS2, Line 289: Append
> nit: "Appends"
Done


PS2, Line 303:  if the transfer is for an outbound call.
> not quite clear here which part of the sentence the "if ..." applies to. If
Thanks for the suggestion. Done.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 29: #include "kudu/rpc/outbound_call.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


Line 30: #include "kudu/rpc/connection.h"
> fix sort order
Done


PS2, Line 260: shutted
> nit: "shut"
Done


Line 266:   // fall-through if remote supports parsing footer.
> we have a special macro here 'FALLTHROUGH_INTENDED' which turns into someth
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS2, Line 159: send_footer
> is this out of date? I don't see this param
Done


Line 171:   // to indicate that the call has been aborted.
> worth indicating whether this call requires that AppendFooter() has previou
Done


PS2, Line 210: IsBeingSent
> Lol. I considered using IsOnTransferQueue() at some point.
Actually, I now recall why I ruled against using that name as it can easily 
mislead readers into believing that the call is in ON_OUTBOUND_QUEUE state. 
Renamed to IsInTransmission() ?


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

PS2, Line 274: Note that the footer is designed to be modified
 :   // after the initial serialization and it will be 
re-serialized after modification.
 :   // To avoid unexpected change in the total message size, keep 
to using fixed sized
 :   // fields only in the footer.
> move this comment to be below the 'aborted' field, since it documents any n
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/serialization.h
File src/kudu/rpc/serialization.h:

Line 78: void IncrementMsgLength(size_t inc_len,
> warning: function 'kudu::rpc::serialization::IncrementMsgLength' has a defi
Done


Line 78: void IncrementMsgLength(size_t inc_len,
> please fix this nit
Done


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

Line 78: /* Initialize the dummy buffer with a known pattern for easier 
debugging. */
> style: // C++ style comment
Done


Line 190:   ++n_payload_slices_;
> how about a post-increment on the prior line?
Done


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

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(4 comments)

Thanks again for taking a look. Replies to some questions below before 
addressing the rest of the comments.

http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 251:   car->transfer->Abort(status);
> what about in the case of talking to an old server which doesn't support fo
Good point. We could also reach this point because negotiation has not 
completed yet. It seems always safe to always call Abort() if negotiation 
hasn't completed yet as we shouldn't have started sending anything yet. Once 
negotiation has completed and the remote doesn't support footer, yes, then this 
should be a no-op for cancellation.

It slightly more complicated for the time-out case if the remote doesn't 
support footer. We cannot invoke OutboundCall::SetTimedOut() right away in 
HandleOutboundCallTimeout() below if the call is mid-transmission or we will 
hit the original bug again. We probably need to leave a status or bool 
indicating that the call has timed out and in OutboundCall::SetSent(), we need 
to invoke the SetTimedOut() instead of waiting for the response.

Please let me know if you think the above proposal is too complicated.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 201: negotiation_complete_ &&
> is negotiation_complete_ necessary? before negotiation is complete, isn't r
I need this to avoid a TSAN warning about concurrent access to remote_features_ 
between the negotiation thread and the reactor thread.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS2, Line 210: IsBeingSent
> hrm, I actually am going to disagree with my previous comment and still thi
Lol. I considered using IsOnTransferQueue() at some point.


http://gerrit.cloudera.org:8080/#/c/7599/2/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:

Line 223:   // the transfer if only the footer is left.
> could this last bit result in a "hung" cancellation in the case that the se
Actually, in this case, we will still invoke OutboundCall::Cancel() right away 
in ReactorThread::CancelOutboundCall() after calling into this function via 
Connection::CancelOutboundCall().

It's true that the OutboundCall object may still hang around for a while 
because the TransferCallback still has a reference to it. Please let me know if 
I may have misunderstood your comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 2:

(33 comments)

http://gerrit.cloudera.org:8080/#/c/7599/1//COMMIT_MSG
Commit Message:

PS1, Line 9: call times out
> worth specifying "times out after transmission of the request has begun but
Done


Line 17: of the sidecars with the RPC layer. This has caused some life cycle
> mind linking to the Impala JIRA where it was discovered this lifecycle was 
Done


PS1, Line 20: fixes the
> not in-flight but mid-transmission. We still don't support a preemptive "ab
Done


PS1, Line 22: . It c
> nit: again I think "mid-transmission" is clearer since we usually call an R
Done


http://gerrit.cloudera.org:8080/#/c/7599/1/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:

PS1, Line 274: | RPC Footer protobuf length (variable encoding) | --- Exists 
only in client requests.
 : ++
 : | RPC Footer protobuf| --- Exists 
only in client requests.
 : +
> This being a variable-length protobuf is a little bit risky, in the sense t
Good point. Doesn't the RPC header have the same problem but I suppose there is 
currently no code which changes the header after serializing it ?

I will add a documentation to state that the footer must only have fixed size 
fields and DCHECK the modified footer doesn't change the total size.


PS1, Line 366: cancelled mid-transmission.
> should clarify "mid-tranmission" here
Done


Line 387:"\x0c"  Request parameter varint: 12 bytes
> can you add some note here that says this capture does _not_ include the fo
Added footer to this message. Also updated the output for RequestHeader and the 
decoding command.


http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

Line 240:   DCHECK(reactor_thread_->IsCurrentThread());
> can you DCHECK that it's the reactor thread here just for documentation val
Done


Line 448: DCHECK_EQ(call_from_map, call_.get());
> warning: parameter 'status' is unused [misc-unused-parameters]
Done


Line 574: 
> I think this should probably be VLOG(2) and need a lot more context to be u
Changed the log level to 2.

I added some more details in Status::Aborted() returned from 
InboundCall::ParseFrom(...).


PS1, Line 649:   const set& required_features = 
car->call->required_rpc_features();
 :   if (!includes(remote_features_.begin(), remote_features_.end(),
 : required_features.begin(), 
required_features.end())) {
 : Status s = Status::NotSupported("server does not support the 
required RPC features");
 : transfer->Abort(s);
 : car->call->SetFailed(s, Phase::REMOTE_CALL);
 : // Test cancellation when 'call_' is in 'FINISHED_ERROR' 
state.
 : MaybeInjectCancellation(car->call);
 : car->call.reset();
 : return false;
 :   }
 : 
 :   // The transfer is ready to be sent. Append a footer if the 
remote system supports it.
 :   // It has to be done here because remote features cannot be 
determined until negotiation
 :   // completes. We know for sure that negotiation has completed 
when we reach this point.
 :   if (RemoteSupportsFooter()) {
 : transfer->AppendFooter(car->call);
 :   }
 :   car->call->SetSending();
 :   // Test cancellation when 'call_' is in 'SENDING' state.
 :   MaybeInjectCancellation(car->call);
 :   return true;
 : }
 : 
 : void Connection::OutboundQueuePopFront() {
 :   OutboundTransfer *transfer = &(outbound_transfers_.front());
 :   // Remove all references to 'transfer' before deleting it.
 :   if (transfer->is_for_outbound_call()) {
 : CallAwaitingResponse* car = FindOrDie(awaiting_response_, 
transfer->call_id());
 : car->tr
> do you think we could refactor out these ~30 lines into a new function? Thi
Good idea. The code looks cleaner after the refactoring.


http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

Line 198:   // Returns true if the remote end has feature flag 
"REQUEST_FOOTERS". Always returns false
> I think a better name is something like 'RemoteSupportsFooters' or somethin
Done


Line 199:   // before negotiation completes. Callers should takes this into 
account.
> use ContainsKey from map-util.h
Done. Also added negotiation_complete_ &&


http://gerrit.cloudera.org:8080/#/c/7599/1/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:

PS1, Line 228: 
 :   RequestFooterPB footer_;
> no 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has uploaded a new change for review.

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

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..

KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Previously, when an outbound call times out, the RPC layer will
continue to send the entire payload (including any sidecars) to
the remote destination until the promised number of bytes specified
in the header are sent. This is problematic for the users of the
RPC layer as there is no well defined point in which the sidecars
are no longer referenced by the RPC layer. The only model in which
this would work is for the caller to either transfer or share ownership
of the sidecars with the RPC layer. This is not always desirable from
the caller's point of view for resource management.

This change fixes the problem above by modifying the RPC protocol
to allow an in-flight RPC call to be aborted. Specifically, a new
footer is added to all outbound call requests. It contains a flag,
when true, indicates the request was cancelled mid-flight and the
inbound call should be ignored altogether. This footer enables
the caller to relinquish references to the sidecars early when
an outbound call is cancelled or timed-out. Once the call is cancelled
or timed-out, the RPC layer will send the remainder of the bytes for
the request with some dummy bytes. Since the aborted flag is set in
the footer, the inbound call will be ignored anyway so it's okay to
send random bytes.

To accommodate the new footer, a new RPC feature flag HAS_FOOTER
is also introduced in this change.

Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
---
M docs/design-docs/rpc.md
M src/kudu/rpc/blocking_ops.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/constants.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/serialization.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
15 files changed, 328 insertions(+), 99 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7455/5/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

PS5, Line 970: 0x1337)
> can you use the return value of SeedRandom() from test_util.cc? That will m
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..

KUDU-2065: Support cancellation for outbound RPC call

This change implements a new interface RpcController::Cancel()
which takes a RpcController as argument and cancels any
pending OutboundCall associated with it.

RpcController::Cancel() queues a cancellation task scheduled
on the reactor thread for that outbound call. Once the task
is run, it will cancel the outbound call right away if
the RPC hasn't started sending yet or if it has already
sent the request and waiting for a response. If cancellation
happens when the RPC request is being sent, the RPC will
be cancelled only after the RPC has finished sending the
request. If the RPC is finished, the cancellation will
be a no-op.

Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
---
M src/kudu/common/wire_protocol.proto
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
M src/kudu/rpc/rtest.proto
16 files changed, 444 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/7455/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-1865: Avoid heap allocation for payload slices
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7471/3/src/kudu/rpc/inbound_call.cc
File src/kudu/rpc/inbound_call.cc:

Line 185:   (*slices)[1] = Slice(response_msg_buf_);
> nit: the () here are not necessary, are they?
Done


Line 187: (*slices)[i+2] = outbound_sidecars_[i]->AsSlice();
> style: can you split this back onto multiple lines with braces? we rarely u
Done


http://gerrit.cloudera.org:8080/#/c/7471/3/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 117: (*slices)[i+2] = sidecars_[i]->AsSlice();
> same
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1865: Avoid heap allocation for payload slices
..

KUDU-1865: Avoid heap allocation for payload slices

As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.

Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 59 insertions(+), 48 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/7471/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..

KUDU-2065: Support cancellation for outbound RPC call

This change implements a new interface RpcController::Cancel()
which takes a RpcController as argument and cancels any
pending OutboundCall associated with it.

RpcController::Cancel() queues a cancellation task scheduled
on the reactor thread for that outbound call. Once the task
is run, it will cancel the outbound call right away if
the RPC hasn't started sending yet or if it has already
sent the request and waiting for a response. If cancellation
happens when the RPC request is being sent, the RPC will
be cancelled only after the RPC has finished sending the
request. If the RPC is finished, the cancellation will
be a no-op.

Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
---
M src/kudu/common/wire_protocol.proto
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
M src/kudu/rpc/rtest.proto
16 files changed, 444 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/7455/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7455/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

Line 966:   // The payload to be used during the RPC.
> can you introduce a constant for this payload size since the magic number 2
Done


Line 982: int idx;
> maybe use a Random instance here? (util/random.h) to randomize the sleep be
Done


http://gerrit.cloudera.org:8080/#/c/7455/4/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

PS4, Line 227:  This function should only be
 :   // called when there is an outstanding ou
> I think this doc should be clarified a bit to include the following importa
Thanks for pointing them out. They are indeed important details to document.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS3, Line 280: MayBe
> nit: "Maybe" is one word rather than "MayBe"
Done


http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:

Line 448:   if (controller.call_) {
> under what circumstance would controller not have a 'call_' member set? Wou
Done


http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

PS3, Line 199: const RpcController&
> I think taking this by pointer makes more sense since it "mutates" the stat
This interface is removed in the latest patch.


http://gerrit.cloudera.org:8080/#/c/7455/3/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS3, Line 264: triggeres
> typo
Done


PS3, Line 266: reference
> typo: references
Done


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/proxy.cc
File src/kudu/rpc/proxy.cc:

Line 106: << "It is illegal to call set_user_credentials() after request 
processing has started";
> yea, it's a little messy vs having it directly in RpcController. Did you co
Implemented RpcController::Cancel() in the latest patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..

KUDU-2065: Support cancellation for outbound RPC call

This change implements a new interface RpcController::Cancel()
which takes a RpcController as argument and cancels any
pending OutboundCall associated with it.

RpcController::Cancel() queues a cancellation task scheduled
on the reactor thread for that outbound call. Once the task
is run, it will cancel the outbound call right away if
the RPC hasn't started sending yet or if it has already
sent the request and waiting for a response. If cancellation
happens when the RPC request is being sent, the RPC will
be cancelled only after the RPC has finished sending the
request. If the RPC is finished, the cancellation will
be a no-op.

Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
---
M src/kudu/common/wire_protocol.proto
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
M src/kudu/rpc/rtest.proto
16 files changed, 425 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/7455/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1865: Avoid heap allocation for payload slices
..

KUDU-1865: Avoid heap allocation for payload slices

As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.

Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 56 insertions(+), 49 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-1865: Avoid heap allocation for payload slices
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7471/1/src/kudu/rpc/inbound_call.h
File src/kudu/rpc/inbound_call.h:

PS1, Line 125: Slice slices[TransferLimits::kMaxPayloadSlices]
> I think using 'std::array* slices' would be a bit
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1865: Avoid heap allocation for payload slices
..

KUDU-1865: Avoid heap allocation for payload slices

As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.

Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 55 insertions(+), 48 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1865: Avoid heap allocation for payload slices

[Michael Ho (Code Review)]

Michael Ho has uploaded a new change for review.

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

Change subject: KUDU-1865: Avoid heap allocation for payload slices
..

KUDU-1865: Avoid heap allocation for payload slices

As shown in KUDU-1865, the heap allocation for the temporary
vector for the slices for holding the serialized payload is
introducing measurable overhead under heavy load. This change
replaces the heap allocation with a stack allocation of an
array of size TransferLimits::kMaxPayloadSlices. With this
change, we saw 10%~15% improvement under heavy workload.

Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
8 files changed, 56 insertions(+), 47 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4470d34ba48db5edaeb66d9e739e0c8942004d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 

[kudu-CR] KUDU-2065: Support cancellation for outbound RPC call

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065: Support cancellation for outbound RPC call
..


Patch Set 2:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/7455/2//COMMIT_MSG
Commit Message:

PS2, Line 9: Proxy::Cancel()
> seems a bit odd to have it in the Proxy rather than just on the controller 
Removed this interface and now use the messenger's interface directly.


http://gerrit.cloudera.org:8080/#/c/7455/2/java/kudu-client/src/main/java/org/apache/kudu/client/Status.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Status.java:

Line 242:   public static Status Cancelled(String msg) {
> I think we should use Aborted here rather than adding a new one. Adding a n
Done


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

PS2, Line 265: FindOrNull
> you can use FindPtrOrNull here to avoid the double-pointer
Done


PS2, Line 296: } else if (call_->cancellation_requested()) {
 :   call_->SetCancelled();
 :  
> in this case I think it would make sense to call SetSent() and _then_ call 
Done. Also moved the check into SetSent().


Line 627: if (car->call.get() == nullptr) {
> see tidy warning -- we prefer to just use the implicit bool cast on smart p
Done


PS2, Line 631: "already timed out")
> can you update this message to be more accurate? I don't think it would eve
Done


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

PS2, Line 280: Try
> I think 'Maybe' is better than 'Try' (try makes me think of something like 
Done


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

PS2, Line 52: debug build
> I don't think you need to be specific to debug builds, it's good to test fa
Done


Line 370: std::lock_guard l(lock_);
> worth a DCHECK(!IsFinished()) here I think
Done


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS2, Line 160: waiting
> "is waiting"
Done


PS2, Line 160: finished
> insert "has"
Done


Line 163:   void Cancel();
> can you add something like: "REQUIRES: must be called from the reactor thre
Done


PS2, Line 185: client
> the client
Done


Line 233: return cancellation_requested_;
> is this always accessed by the reactor thread? if not, does it need some sy
Yes but there is no easy way to DCHECK it here. Comments added.


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/proxy.cc
File src/kudu/rpc/proxy.cc:

Line 106: messenger_->QueueCancellation(controller->call_);
> I commented elsewhere that I don't think this is great to have in the Proxy
Yes, the issue is that the RpcController doesn't really have a handle to the 
messenger.

I deleted this interface and added an interface Messenger::Cancel(const 
RpcController&). I had the impression from the comment in Messenger class that 
it's breaking the abstraction to pass RpcController to Messenger directly but 
actually sounds okay based on your comment.


http://gerrit.cloudera.org:8080/#/c/7455/2/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:

PS2, Line 242: is
> as
Done


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

Line 890: TEST_P(TestRpc, TestCancellation) {
> this is a nice test, but would be good to also add more of a "stress test" 
Good idea. Working on it now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf53c5b113de10d573bd32fb9b2293572e806fbf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes