[Impala-ASF-CR] IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8510 )

Change subject: IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow
..


Patch Set 2:

Thanks for doing this. It seems reasonable to me to switch to CTR.

One concern I have is were you able to test this on a system with OpenSSL 
1.0.0? Their documentation isn't clear, so I'm not sure if this API is 
supported in that version.
The best I could see was this line of code that was under an #if 0:
https://github.com/openssl/openssl/blob/OpenSSL_1_0_0-stable/crypto/evp/evp.h#L782-L783

It would be great if you could verify that.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib97939f2334838263364b53ef3413871638bf53e
Gerrit-Change-Number: 8510
Gerrit-PatchSet: 2
Gerrit-Owner: Xianda Ke 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mike Yoder 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:27:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

[Sailesh Mukil (Code Review)]

Hello Michael Ho,

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

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

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

Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
..

IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

KuduRPC has support for Kerberos. However, since Impala's client transport
still uses the Thrift transport stack, we need to make sure that a single
security configuration applies to both internal communication (KuduRPC)
and external communication (Thrift's TSaslTransport).

This patch changes InitAuth() to start Sasl regardless of security
configuration, since KRPC uses plain SASL for negotiation on insecure
clusters.

It also moves some utility code out of authentication.cc into
auth-util.cc for resuse by the RpcMgr while enabling kerberos.

The MiniKDC related code is moved out of thrift-server-test.cc into a
new file called mini-kdc-wrapper.h/cc. This file exposes a new class
MiniKdcWrapper which can be easily used by the tests to configure the
kerberos environment, create the keytab, start the KDC and also
initialize the Impala security library.

Tests are added to rpc-mgr-test for kerberos tests over KRPC.
thrift-server-test also has a mechanical change to use MiniKdcWrapper.
Also tested on a live cluster configured to use kerberos.

Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication-test.cc
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/CMakeLists.txt
A be/src/testutil/mini-kdc-wrapper.cc
A be/src/testutil/mini-kdc-wrapper.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
12 files changed, 438 insertions(+), 172 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/3
--
To view, visit http://gerrit.cloudera.org:8080/8270
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
Gerrit-Change-Number: 8270
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8270 )

Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
..


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/rpc/rpc-mgr-test.cc@124
PS2, Line 124:
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/rpc/rpc-mgr.cc@66
PS2, Line 66:
> nit: long line.
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h
File be/src/testutil/mini-kdc-wrapper.h:

http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h@18
PS2, Line 18: WRAPPE
> WRAPPER ?
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h@40
PS2, Line 40: mode
> mode
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h@65
PS2, Line 65:   const std::string spn_;
> const ? Same below.
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h@66
PS2, Line 66:
> nit: white space.
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h@75
PS2, Line 75:
> nit: white space.
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/testutil/mini-kdc-wrapper.h@82
PS2, Line 82: // Starts the KDC and configures it
> One line comment.
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/util/auth-util.h
File be/src/util/auth-util.h:

http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/util/auth-util.h@47
PS2, Line 47: internal kerberos principal.
> Mind adding comments explaining what the difference is between internal and
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/util/auth-util.h@50
PS2, Line 50: string.
> Same comment as GetInternalKerberosPrincipal().
Done


http://gerrit.cloudera.org:8080/#/c/8270/2/be/src/util/auth-util.h@54
PS2, Line 54:
> Please also document the return value. For example, what if 'principal' doe
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
Gerrit-Change-Number: 8270
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 10 Nov 2017 00:08:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6164: Fix stale query profile in TestAlwaysFalseFilter

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8498 )

Change subject: IMPALA-6164: Fix stale query profile in TestAlwaysFalseFilter
..


Patch Set 1: Code-Review+1

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8498/1//COMMIT_MSG@13
PS1, Line 13: finalization is performed in Coordinator::GetNext. A bug related 
to
nit: Add "after we hit 'eos'"


http://gerrit.cloudera.org:8080/#/c/8498/1//COMMIT_MSG@13
PS1, Line 13: A bug related to
: decoding query profile is also fixed.
Could you briefly mention what the noticeable implications were of this bug? It 
will be good for future reference.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I04bb76d20541fa035d88167b593d1b8bc3873e89
Gerrit-Change-Number: 8498
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 08 Nov 2017 23:12:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5473: [DOCS] Document TLS min version & cipher options

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8401 )

Change subject: IMPALA-5473: [DOCS] Document TLS min version & cipher options
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1705262f8c01e38c616541d1c48f5d0cad5498e
Gerrit-Change-Number: 8401
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 07 Nov 2017 17:05:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3613: Avoid topic updates to unregistered subscriber instances

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8449 )

Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber 
instances
..


Patch Set 5: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65
Gerrit-Change-Number: 8449
Gerrit-PatchSet: 5
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 07 Nov 2017 00:29:24 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4671: (part-1) Copy kudu::ServicePool into Impala namespace

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8471 )

Change subject: IMPALA-4671: (part-1) Copy kudu::ServicePool into Impala 
namespace
..

IMPALA-4671: (part-1) Copy kudu::ServicePool into Impala namespace

*** THIS PATCH IS NOT FOR REVIEW. IT IS JUST UPLOADED TO MAKE THE
REVIEW OF PART-2 EASIER IN GERRIT BY SHOWING ONLY THE DIFFS OF THAT
PATCH AGAINST THIS PATCH. MORE EXPLANATION BELOW***

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 1 of the second option. In this patch, we simply copy
the kudu::ServicePool code into be/src/rpc/.

In the next patch (Part 2), we rename it to ImpalaServicePool,
and we do the instrumentation such that it shows up on the
webpage.

We split it up this way into 2 parts so that it will be easy to review.

Change-Id: I5bb927d4726d4fe418251b9734a4e232810fef69
---
A be/src/rpc/impala-service-pool.cc
A be/src/rpc/impala-service-pool.h
2 files changed, 317 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/71/8471/3
--
To view, visit http://gerrit.cloudera.org:8080/8471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bb927d4726d4fe418251b9734a4e232810fef69
Gerrit-Change-Number: 8471
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala threads

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
..

IMPALA-4671: (part-2) Replace kudu::ServicePool with one that uses Impala 
threads

The KuduRPC subsystem uses kudu::ServicePool to service all incoming
RPCs. Since this lives inside the Kudu codebase, all instrumentation
of RPCs flowing through the KuduRPC subsystem is not visible to Impala,
as Kudu does not export this instrumentation, but only maintains it
internally within kudu::ServicePool.

In order to reliably view the instrumentation of all RPCs flowing through
KRPC, one option is to modify kudu::ServicePool to take their
instrumentation and display it on our webpages, which is very invasive.

A second option is to have a parallel implementation of kudu::ServicePool
which for all purposes behaves the same way, but instead has this extra
code that displays this instrumentation in our webpages.

This patch is Part 2 of the second option.

In Part 1, we simply copied the code from kudu::ServicePool into the Impala
namespace. In this patch, we rename the redundant kudu::ServicePool
(that was copied into be/src/rpc/) to ImpalaServicePool. We also expose
the instrumentaion of the ImpalaServicePool as JSON.

This patch also enables the tracking of the RpcMgr via the Webserver. The
RpcMgr calls into the active ImpalaServicePool objects and retrieves their
instrumentation as JSON and makes it available in the Impala webpage.

Finally, the threads in use by the service pool will also be visible in
the /threadz page.

A screenshot of the new additions to the /rpcz page is here:
https://ibb.co/nciLDG

This patch is partially based on an abandoned patch by Henry Robinson.

Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/service/impalad-main.cc
M www/rpcz.tmpl
9 files changed, 411 insertions(+), 139 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/3
--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-3613: Avoid topic updates to unregistered subscriber instances

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8449 )

Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber 
instances
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@383
PS2, Line 383: d subscriber
> >  Where ever the 'SubscriberId' is
Thanks for the explanation.

Yea my point was if we're going to have a unique RegistrationId anyway, why 
have a SubscriberId. It seemed redundant.

But as you pointed out, it looks like the subscriber chooses the subscriber_id 
and not the statestore. So, it would be hard to enforce this.

Let's leave this for now.


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc@414
PS2, Line 414: onst SubscriberId& subscriber_id,
 : const TUniqueId& registration_id
> Not sure I understand. We get the subscriber/registration_id from the Sched
Nvm, my bad, I thought both were coming from the Subscriber object. Ignore this.


http://gerrit.cloudera.org:8080/#/c/8449/3/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

http://gerrit.cloudera.org:8080/#/c/8449/3/be/src/statestore/statestore.cc@278
PS3, Line 278:   lock_guard l(subscribers_lock_);
 :   lock_guard t(topic_lock_);
I just noticed this. Getting a SpinLock before getting a mutex is an 
anti-pattern.

Even attempting to get a spinlock while already holding a spinlock is also not 
exactly a great idea. However, our SpinLock implementation sleeps after a few 
cycles of trying to obtain the lock anyway.

Do we know if we do a lot of work holding the topic_lock_? If not, let's change 
this to a SpinLock too. (The GatherTopicUpdates() holds topic_lock_ and 
iterates through a nested loop, but I'm not sure how many iterations that would 
be in the worst case).

If it looks like we will end up doing a lot of work holing the lock, we can be 
safe and just turn the 'subscribers_lock_' back to a mutex.


http://gerrit.cloudera.org:8080/#/c/8449/3/be/src/statestore/statestore.cc@415
PS3, Line 415: const TUniqueId&
const RegistrationId&



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65
Gerrit-Change-Number: 8449
Gerrit-PatchSet: 3
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sat, 04 Nov 2017 00:18:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6144: PublishFilter() continues to run after query failure/cancellation

[Sailesh Mukil (Code Review)]

Hello Bharath Vissapragada, Tim Armstrong,

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

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

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

Change subject: IMPALA-6144: PublishFilter() continues to run after query 
failure/cancellation
..

IMPALA-6144: PublishFilter() continues to run after query failure/cancellation

The CoordinatorBackendState::PublishFilter() function does not check
for query failure/cancellation. So if runtime filters are being published
during/after a failure, they will not be cancelled and still be sent
out which may take a while depending on the size of the cluster.
Also, these functions could potentially hold very large amounts
of untracked memory.

This patch fixes it by checking for cancellation/failure in
PublishFilter.

Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
2 files changed, 9 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8455/2
--
To view, visit http://gerrit.cloudera.org:8080/8455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6144: PublishFilter() continues to run after query failure/cancellation

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8455 )

Change subject: IMPALA-6144: PublishFilter() continues to run after query 
failure/cancellation
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc@394
PS1, Line 394:   {
> same
Done


http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc@394
PS1, Line 394:   {
> Also, should we check if the backend is done too?
Done


http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1082
PS1, Line 1082:   TPublishFilterParams rpc_params;
> lock_?
I removed this check since the check in L1106 should be sufficient.


http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1106
PS1, Line 1106: if (filter_updates_received_->value() == 0) {
> I think checking for the filter disabled is sufficient in this function, si
Good point. I removed the above check. Plus acquiring the lock above to check 
the status would be very expensive.


http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1130
PS1, Line 1130: swap(rpc_params.bloom_filter, aggregated_filter);
> I think we should also track the rpc_params memory and prevent the query fr
Sure I opened IMPALA-6153 and linked it to IMPALA-1575.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 03 Nov 2017 20:13:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8455


Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run 
after query failure/cancellation
..

IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query 
failure/cancellation

The Coordinator::UpdateFilter() and Coordinator::PublishFilter()
functions do not check for query failure/cancellation. So if
runtime filters are being aggregated/published during a failure,
they will not be cancelled and still be sent out which may take
a while depending on the size of the cluster. Also, these functions
could potentially hold very large amounts of untracked memory.

This patch fixes it by checking for cancellation in both the
functions.

Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
2 files changed, 3 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8455/1
--
To view, visit http://gerrit.cloudera.org:8080/8455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-3613: Avoid topic updates to unregistered subscriber instances

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8449 )

Change subject: IMPALA-3613: Avoid topic updates to unregistered subscriber 
instances
..


Patch Set 2:

(7 comments)

Thanks for doing this patch. This will help reduce a lot of unnecessary network 
traffic.

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore-subscriber.h
File be/src/statestore/statestore-subscriber.h:

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore-subscriber.h@165
PS2, Line 165:   typedef TUniqueId RegistrationId;
You can move this typedef to statestore.h and use the same type in 
statestore.h/cc too.


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@383
PS2, Line 383: SubscriberId
Do we even need to store the SubscriberId here? Can't we just store a unique 
registration ID? Where ever the 'SubscriberId' is required, we just get it from 
the Subscriber object anyway, and that object can be retrieved using the unique 
registration id.


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@481
PS2, Line 481: subscriber exists
Could you clarify what "exists" means here exactly? It could be confused with a 
node just existing as a part of the cluster. I think we want to say that it 
exists in the subscribers_ map.


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@482
PS2, Line 482: registration_ids
nit: registration_id


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.h@484
PS2, Line 484: std::shared_ptr* subscriber
Add a comment about what is returned in this out parameter.


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc@414
PS2, Line 414: onst SubscriberId& subscriber_id,
 : const TUniqueId& registration_id
It looks like it just makes sense to pass 'const Subscriber&' here? Is there a 
case where we would not get a subscriber_id and a registration_id from the same 
Subscriber object while calling this function?


http://gerrit.cloudera.org:8080/#/c/8449/2/be/src/statestore/statestore.cc@634
PS2, Line 634:   if (!RegisteredSubscriberExists(subscriber_to_update.first, 
subscriber_to_update.second,
I'm a little worried that we're contending for a mutex two more times in this 
function. Do you anticipate any performance regression due to increased context 
switching?

Consider using a spin lock if we won't have more than ~1000 entries in the map 
at one time. (unordered_map has a worst-case O(N) time complexity)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0329ae7d23dc6e9b04b7bc3ee8d89cbc73756f65
Gerrit-Change-Number: 8449
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 02 Nov 2017 03:19:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5054: [SECURITY] Enable KRPC w/ TLS in Impala

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8439


Change subject: IMPALA-5054: [SECURITY] Enable KRPC w/ TLS in Impala
..

IMPALA-5054: [SECURITY] Enable KRPC w/ TLS in Impala

KRPC has some flags that turn on TLS. This patch sets those to enable
TLS communication.

Tests are added to rpc-mgr-test.

Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
---
M be/src/catalog/catalogd-main.cc
M be/src/rpc/authentication-test.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/runtime/exec-env.cc
M be/src/service/impala-server.cc
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore.cc
M be/src/statestore/statestored-main.cc
M be/src/testutil/in-process-servers.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
15 files changed, 366 insertions(+), 37 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8439/1
--
To view, visit http://gerrit.cloudera.org:8080/8439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

[Sailesh Mukil (Code Review)]

Hello Michael Ho,

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

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

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

Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
..

IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

KuduRPC has support for Kerberos. However, since Impala's client transport
still uses the Thrift transport stack, we need to make sure that a single
security configuration applies to both internal communication (KuduRPC)
and external communication (Thrift's TSaslTransport).

This patch changes InitAuth() to start Sasl regardless of security
configuration, since KRPC uses plain SASL for negotiation on insecure
clusters.

It also moves some utility code out of authentication.cc into
auth-util.cc for resuse by the RpcMgr while enabling kerberos.

The MiniKDC related code is moved out of thrift-server-test.cc into a
new file called mini-kdc-wrapper.h/cc. This file exposes a new class
MiniKdcWrapper which can be easily used by the tests to configure the
kerberos environment, create the keytab, start the KDC and also
initialize the Impala security library.

Tests are added to rpc-mgr-test for kerberos tests over KRPC.
thrift-server-test also has a mechanical change to use MiniKdcWrapper.
Also tested on a live cluster configured to use kerberos.

Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication-test.cc
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/CMakeLists.txt
A be/src/testutil/mini-kdc-wrapper.cc
A be/src/testutil/mini-kdc-wrapper.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
12 files changed, 427 insertions(+), 172 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/2
--
To view, visit http://gerrit.cloudera.org:8080/8270
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
Gerrit-Change-Number: 8270
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4978 / IMPALA-5631: [DOCS] Add FQDN known issue

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7388 )

Change subject: IMPALA-4978 / IMPALA-5631: [DOCS] Add FQDN known issue
..


Patch Set 3:

(1 comment)

Sorry, just saw this today.

http://gerrit.cloudera.org:8080/#/c/7388/3/docs/topics/impala_known_issues.xml
File docs/topics/impala_known_issues.xml:

http://gerrit.cloudera.org:8080/#/c/7388/3/docs/topics/impala_known_issues.xml@163
PS3, Line 163: Depends on the resolution of IMPALA-4978
I've punted on IMPALA-4978 because I was afraid that there may be some edge 
cases that we may regress on and never got back to it.

But it's worth adding here that even if IMPALA-4978 is fixed, there will be 
cases where it's not possible to get the FQDN via getaddrinfo().

So, fixing IMPALA-4978 will of course alleviate this problem, but the problem 
may still show up in certain cases. In those cases, users still need to fall 
back to this workaround. I think this is something we want to add.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib039d0102878f1c05470371f581cb258287b9bc0
Gerrit-Change-Number: 7388
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:34:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8412 )

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..


Patch Set 5: Code-Review+2

Thanks for the review Tim. I added a function header comment for InitAuth() 
documenting this.

Rebase. Carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:21:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..

IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.

Change-Id: I2cd434757de2cd384def5b360a479e51812a
---
M be/src/rpc/auth-provider.h
M be/src/rpc/thrift-server-test.cc
2 files changed, 19 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/4
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8412 )

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..


Patch Set 3:

Looks like I found the real bug. The SASL library takes the string and holds a 
reference to it instead of copying it in sasl_server_init().

However, when we reinitialize the SASL library, it doesn't take in the new 
string because it detects that it was already previously initialized:
https://github.com/cyrusimap/cyrus-sasl/blob/master/lib/server.c#L841

And we end up discarding the string that was held by it.

So the fix is to get the string once and make sure it lives as long as the 
process does.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 31 Oct 2017 17:00:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..

IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.

Change-Id: I2cd434757de2cd384def5b360a479e51812a
---
M be/src/rpc/thrift-server-test.cc
1 file changed, 15 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/3
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8412 )

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8412/2/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8412/2/be/src/rpc/thrift-server-test.cc@111
PS2, Line 111: string current_executable_path;
> Correct me if I'm wrong, but it seems like the root of the problem is that
After moving the string into InitAuth() and storing it in AuthManager (which is 
a global singleton), the ASAN error still kept showing up. I spent many hours 
trying to debug this problem and I have no understanding of why it happens. My 
inclination is that ASAN gets confused by how 
kudu::PosixEnv::GetExecutablePath() creates a string. It creates a 
unique_ptr and then assign()'s it to a string.

I see no reason why your suggestion shouldn't work, but for some reason this is 
seen as a heap-use-after free if it is called multiple times.

The only thing that works for now is to leave the code as is in this patch set.

I have 2 attempts to move/copy the string into GetExecutablePath() and their 
corresponding errors here:
https://github.com/smukil/incubator-impala/commit/d3fedb0d665def4870b787a586e922e361cac2dc
 (copy)
https://github.com/smukil/incubator-impala/commit/c2e6ca8987054539be70e5ef7e3244c49df2ee40
 (move)

Opened IMPALA-6132 to track this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 31 Oct 2017 05:46:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..

IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.

Change-Id: I2cd434757de2cd384def5b360a479e51812a
---
M be/src/rpc/thrift-server-test.cc
1 file changed, 4 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/2
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8412 )

Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8412/1/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8412/1/be/src/rpc/thrift-server-test.cc@109
PS1, Line 109: string current_executable_path;
> Brief comment why this needs to be here. Alternative: static member in the
Added a comment.

Adding this to the ThriftParamsTest will not fix this issue since the 
InitAuth() code maintains process wide state and does not have a shutdown 
mechanism.

So, the threads started by it will keep running in the background even when 
~ThriftParamsTest() is called, leading to the possibility of the same 
use-after-free bug.

This is not ideal, but for the Impala process we need to start the 
authentication code only once. For the purpose of testing, we need to add a 
clean shutdown mechanism, so that we can start and stop the auth library for 
each test case. Right now we do it only for one test case. This is tracked by 
IMPALA-6085.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 30 Oct 2017 17:35:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8412


Change subject: IMPALA-6126: ASAN detects heap-use-after-free in 
thrift-server-test
..

IMPALA-6126: ASAN detects heap-use-after-free in thrift-server-test

A recent patch for IMPALA-5129 introduced a use-after-free bug in
thrift-server-test. It is fixed in this patch.

Change-Id: I2cd434757de2cd384def5b360a479e51812a
---
M be/src/rpc/thrift-server-test.cc
1 file changed, 2 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/8412/1
--
To view, visit http://gerrit.cloudera.org:8080/8412
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2cd434757de2cd384def5b360a479e51812a
Gerrit-Change-Number: 8412
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5473: [DOCS] Document TLS min version & cipher options

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8401 )

Change subject: IMPALA-5473: [DOCS] Document TLS min version & cipher options
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8401/1/docs/topics/impala_ssl.xml
File docs/topics/impala_ssl.xml:

http://gerrit.cloudera.org:8080/#/c/8401/1/docs/topics/impala_ssl.xml@182
PS1, Line 182:
Could you also please add a note saying that TLSv1.2 may not work on 
CentOS/RHEL 6, even if OpenSSL 1.0.1 is available?

It is due to this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1497859

We are still working on a fix.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1705262f8c01e38c616541d1c48f5d0cad5498e
Gerrit-Change-Number: 8401
Gerrit-PatchSet: 1
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Oct 2017 17:39:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1575: Part 1: eagerly release query exec resources

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8303 )

Change subject: IMPALA-1575: Part 1: eagerly release query exec resources
..


Patch Set 11: Code-Review+1

(1 comment)

Thanks for the explanations. This LGTM.

http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc@1095
PS10, Line 1095: query_state_ != nullptr)
> Sure, so what you are saying is that query_state_ being not null is not an
Makes sense. Better not to make decisions based on non-contractual current 
implementation details that may change at any point. Seems like we have the 
same pattern for a lot of other objects too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I41ff374b0403f10a145f7fee9b3145953ee32341
Gerrit-Change-Number: 8303
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 26 Oct 2017 22:49:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 14: Code-Review+2

Fix minor clang-tidy issues.

Carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 14
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 26 Oct 2017 20:29:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Michael Brown, Jim Apple, Bikramjeet Vig, Dan Hecht, Impala 
Public Jenkins,

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

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

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

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..

IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).

Updated bin/bootstrap_system.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Also fixed a bug that didn't transfer the environment into 'sudo'
in bin/bootstrap_system.sh.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 234 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/14
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 14
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-1575: Part 1: eagerly release query exec resources

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8303 )

Change subject: IMPALA-1575: Part 1: eagerly release query exec resources
..


Patch Set 10:

(4 comments)

Just have a few minor comments.

http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc@1095
PS10, Line 1095: query_state_ != nullptr)
How would query_state_ be NULL if the coordinator hasn't release its reference 
to it yet?

We always create a QueryState for a Coordinator and there's no point of failure 
between when a Coordinator object is created and a QueryState for that 
Coordinator object is created.


http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/mem-tracker.cc@322
PS10, Line 322: while (tracker != nullptr && !tracker->is_query_mem_tracker_) {
  : tracker = tracker->parent_;
  :   }
What's the thread safety story here? What if a any one of the trackers touched 
here are unregistered from their parent in the middle of this loop?


http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc@96
PS10, Line 96:   if (query_mem_tracker_ != nullptr) {
 : // After this point nothing should be touching this query's 
MemTracker and all
 : // tracked memory associated with the query must be 
released. The whole query
 : // subtree of MemTrackers can be removed from the global 
tree and destroyed.
 : query_mem_tracker_->CloseAndUnregisterFromParent();
 :   }
I'm still not clear why this moved from ReleaseExecResources() to 
~QueryState(). Doesn't this mean we're releasing the tracked memory later?


http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/query-state.cc@135
PS10, Line 135: AcquireExecResourceRefcount(); // Decremented in 
QueryExecMgr::StartQueryHelper().
Can we move this to the start of Init() and make L95 a DCHECK?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I41ff374b0403f10a145f7fee9b3145953ee32341
Gerrit-Change-Number: 8303
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 26 Oct 2017 18:37:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh
File bin/bootstrap_system.sh:

http://gerrit.cloudera.org:8080/#/c/7938/12/bin/bootstrap_system.sh@111
PS12, Line 111: if ! { service --status-all | grep -E '^ \[ \+ \]  ssh$'; }
> The apt-get on line 106 is calling the function on line 79. Maybe the probl
Aha, looks like that was the problem. When running sudo apt-get in L79, we 
didn't transfer the environment variables into the sudo scope. I fixed that by 
adding -E. I also tested that it works.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 26 Oct 2017 06:10:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Michael Brown, Jim Apple, Bikramjeet Vig, Dan Hecht, Impala 
Public Jenkins,

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

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

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

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..

IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).

Updated bin/bootstrap_system.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Also fixed a bug that didn't transfer the environment into 'sudo'
in bin/bootstrap_system.sh.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 234 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/13
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 13
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 12:

The GVO was hanging since the kerberos package installations expected user 
input. It turns out that the DEBIAN_FRONTEND=noninteractive option was not 
being respected by these packages for some reason, but doing the same as sudo 
made it work. There's no mention anywhere online of why this works, but just 
other online threads with people who claim that this worked for them too.

I don't like having things around that I can't explain, but unfortunately, I 
don't see any other way to do this now.

@Michael Brown and/or @Jim Apple, could you please review/sign-off on 
bootstrap_system.sh when you get the chance?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 12
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 26 Oct 2017 05:11:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..

IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.
Some of these binaries are installed via sudo to avoid the
interactive dialog screens. If we install without sudo, they do
not respect the 'noninteractive' option and still wait for user
input.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 239 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/12
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 12
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4456: Address scalability issues of qs map lock and client request state map lock

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8363 )

Change subject: IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_
..


Patch Set 4:

(4 comments)

> Feel free to ignore, but I'm curious if you tried out SpinLock too.
 > be/src/benchmarks/lock-benchmark.cc shows that it is somehow more
 > efficient than boost::mutex under contention.

Using SpinLock shows better performance in the general case. So, I've switched 
qs_map_lock_ to use spin locks. I haven't done the same for 
client_request_state_map_lock_, since the access patterns may be different, and 
it would be best to confirm with a benchmark first before making a decision. I 
can add a benchmark in a follow on patch.

http://gerrit.cloudera.org:8080/#/c/8363/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8363/3//COMMIT_MSG@30
PS3, Line 30: sharded to a default of 4 buckets initally.
> nit: sharded
Done


http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/runtime/query-exec-mgr.cc@84
PS3, Line 84: DEFINE_int32(log_mem_usage_interval, 0, "If non-zero, impalad 
will output memory usage "
: "every log_mem_usage_interval'th fragment complet
> Would it make sense to write a helper so that this code could look like:
Yup, that would make more sense. I've added a ScopedQueryStateMap {} class that 
locks/unlocks the appropriate map(bucket) on scope entry/exit respectively.

I added a similar class ScopedCRSMap in impala-server.h/cc.


http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-http-handler.cc@244
PS3, Line 244:   stringstream ss;
> 4 should be the constant here, no?
Yup, changed it.


http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/service/impala-server.h@562
PS3, Line 562:   /// ClientRequestState's deletion. Also writes the query 
profile to the profile log
> nit: corresponding
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
Gerrit-Change-Number: 8363
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 25 Oct 2017 22:48:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4456: Address scalability issues of qs map lock and client request state map lock

[Sailesh Mukil (Code Review)]

Hello Philip Zeyliger, Tim Armstrong,

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

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

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

Change subject: IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_
..

IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_

The following 2 locks have shown to be frequent points of contention
on recent perf runs:

- qs_map_lock_
- client_request_state_map_lock_

Since these are process wide locks, any threads waiting on these locks
potentially slow down the runtime of a query.

I tried to address this previously by converting the 
client_request_state_map_lock_
to a reader-writer lock. This showed great perf improvements in the general
case, however, there were edge cases with big regressions as well.
In the general case, strict readers of the map got through so quickly
that we were able to see a reduction in the number of client connections
created, since this lock was contended for in the context of Thrift threads too.
The bad case is when writers were starved trying to register a new query
since there were so many readers. Changing the starve option resulted in
worse read performance all round.

Another approach which is relatively simpler is to shard the locks, which
proves to be very effective with no regressions. The maps and locks are
sharded to a default of 4 buckets initally.

Query IDs are created by using boost::uuids::random_generator. We use the
high bits of a query ID to assign queries to buckets. I verified that the
distribution of the high bits of a query ID are even across buckets on
my local machine:

For 10,000 queries sharded across 4 buckets, the distribution was:
bucket[0]: 2500
bucket[1]: 2489
bucket[2]: 2566
bucket[3]: 2445

A micro-benchmark is added to measure the improvement in performance. This
benchmark creates multiple threads each of which creates a QueryState and
accesses it multiple times. We can see improvements in the range 2x - 3.5x.

BEFORE:
--Benchmark 1: Create and access Query States.
Total Time (#Queries: 5 #Accesses: 100) : 1ms
Total Time (#Queries: 50 #Accesses: 100) : 8ms
Total Time (#Queries: 50 #Accesses: 1000) : 54ms
Total Time (#Queries: 500 #Accesses: 100) : 76ms
Total Time (#Queries: 500 #Accesses: 1000) : 543ms

AFTER:
--Benchmark 1: Create and access Query States.
Total Time (#Queries: 5 #Accesses: 100) : 2173.59K clock cycles
Total Time (#Queries: 50 #Accesses: 100) : 4ms
Total Time (#Queries: 50 #Accesses: 1000) : 15ms
Total Time (#Queries: 500 #Accesses: 100) : 46ms
Total Time (#Queries: 500 #Accesses: 1000) : 151ms

NOTE: This microbenchmark has shown that SpinLock has better performance
than boost::mutex for the qs_map_lock_'s, so that change has been made
too.

TODO: Add benchmark for client_request_state_map_lock_ too. The APIs
around that are more complicated, so this patch only includes
the benchmarking of qs_map_lock_.

Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/process-wide-locks-benchmark.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/uid-util.h
8 files changed, 338 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/8363/4
--
To view, visit http://gerrit.cloudera.org:8080/8363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
Gerrit-Change-Number: 8363
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4456: Address scalability issues of qs map lock and client request state map lock

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8363 )

Change subject: IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_
..

IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_

The following 2 locks have shown to be frequent points of contention
on recent perf runs:

- qs_map_lock_
- client_request_state_map_lock_

Since these are process wide locks, any threads waiting on these locks
potentially slow down the runtime of a query.

I tried to address this previously by converting the 
client_request_state_map_lock_
to a reader-writer lock. This showed great perf improvements in the general
case, however, there were edge cases with big regressions as well.
In the general case, strict readers of the map got through so quickly
that we were able to see a reduction in the number of client connections
created, since this lock was contended for in the context of Thrift threads too.
The bad case is when writers were starved trying to register a new query
since there were so many readers. Changing the starve option resulted in
worse read performance all round.

Another approach which is relatively simpler is to shard the locks, which
proves to be very effective with no regressions. The maps and locks are
shareded to a default of 4 buckets initally.

Query IDs are created by using boost::uuids::random_generator. We use the
high bits of a query ID to assign queries to buckets. I verified that the
distribution of the high bits of a query ID are even across buckets on
my local machine:

For 10,000 queries sharded across 4 buckets, the distribution was:
bucket[0]: 2500
bucket[1]: 2489
bucket[2]: 2566
bucket[3]: 2445

A micro-benchmark is added to measure the improvement in performance. This
benchmark creates multiple threads each of which creates a QueryState and
accesses it multiple times. We can see improvements in the range 2x - 3.5x.

BEFORE:
--Benchmark 1: Create and access Query States.
Total Time (#Queries: 5 #Accesses: 100) : 1ms
Total Time (#Queries: 50 #Accesses: 100) : 8ms
Total Time (#Queries: 50 #Accesses: 1000) : 54ms
Total Time (#Queries: 500 #Accesses: 100) : 76ms
Total Time (#Queries: 500 #Accesses: 1000) : 543ms

AFTER:
--Benchmark 1: Create and access Query States.
Total Time (#Queries: 5 #Accesses: 100) : 2173.59K clock cycles
Total Time (#Queries: 50 #Accesses: 100) : 4ms
Total Time (#Queries: 50 #Accesses: 1000) : 15ms
Total Time (#Queries: 500 #Accesses: 100) : 46ms
Total Time (#Queries: 500 #Accesses: 1000) : 151ms

TODO: Add benchmark for client_request_state_map_lock_ too. The APIs
around that are more complicated, so this patch only includes
the benchmarking of qs_map_lock_.

Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/process-wide-locks-benchmark.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/uid-util.h
8 files changed, 236 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/8363/3
--
To view, visit http://gerrit.cloudera.org:8080/8363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
Gerrit-Change-Number: 8363
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4456: Address scalability issues of qs map lock and client request state map lock

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8363 )

Change subject: IMPALA-4456: Address scalability issues of qs_map_lock_ and 
client_request_state_map_lock_
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/util/uid-util.h
File be/src/util/uid-util.h:

http://gerrit.cloudera.org:8080/#/c/8363/3/be/src/util/uid-util.h@30
PS3, Line 30: #define NUM_QUERY_BUCKETS 4
It feels a little odd having a constant that's in use in runtime/ and service/, 
defined in util/, but I don't have a better idea. If anyone has a better idea 
as to where to put it, it would be gladly appreciated.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61089090e1095da45a8a64ed3ccc78bd310807f1
Gerrit-Change-Number: 8363
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 23 Oct 2017 22:14:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 11: Code-Review+2

After a rebase, the binaries listed in bootstrap_development moved to 
bootstrap_system, causing all the changes I had in that file to be lost. Hence 
the Jenkins -1.

I just added the binaries to install back to bootstrap_system.

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 23 Oct 2017 18:38:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..

IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M bin/bootstrap_system.sh
A cmake_modules/FindKerberosPrograms.cmake
10 files changed, 233 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/11
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 11
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 10: Code-Review+2

Carrying +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 10
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 20 Oct 2017 20:58:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..


Patch Set 9:

(2 comments)

> > (1 comment)
 >
 > Manually setting Verified: -1.
 >
 > Running the test in a loop shows that it's flaky. Not a functional
 > issue, but something to do with setting up and tearing down the KDC
 > so often. Or possibly a test infra issue because our renew threads
 > don't have a shutdown mechanism.
 >
 > Will work on fixing it.

On more investigation, it looks like the cause of this problem is that our 
security code (and Kudu's security code) does not have a shutdown mechanism. 
So, Init-ing the security library multiple times can have undefined behavior, 
since every time it is init-ed, a new renewal thread runs in the background and 
is never stopped, and these renewal threads have access to common state. Our 
security code is written with the assumption that it will be init-ed only once, 
which holds true for live clusters, but not for test purposes we aim to do in 
this patch.

I've opened IMPALA-6085 to track this to fix it in the long term. In the short 
term, I've changed the code to using kerberos only for one test per process. 
(in this case, the SslConnectivity test).

Documenting the other things I tried to fix this:
- Start and stop the KDC in the main() function: The problem is that we can't 
run it with and without kerberos in this case. Also, Gtest doesn't guarantee 
calling RUN_ALL_TESTS() twice will work, if we want to run with and without 
kerberos.

- Keep around some extra global state to start and stop the KDC according to 
the test we're in. This is possible, but is a big hack, and I'm against 
checking that kind of code in.

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

http://gerrit.cloudera.org:8080/#/c/7938/8//COMMIT_MSG@7
PS8, Line 7: Kudu
> Kudu
Done


http://gerrit.cloudera.org:8080/#/c/7938/8/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/7938/8/be/src/rpc/authentication.cc@109
PS8, Line 109: // (IMPALA-5893)
> can you file a jira to remove this flag (and the old code) within a release
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 19 Oct 2017 20:33:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig, Dan Hecht,

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

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

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

Change subject: IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork
..

IMPALA-5129: Use Kudu's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

TODO: Since the setting up and tearing down of our security code
isn't idempotent, we can run only any one test in a process with
Kerberos now (IMPALA-6085).

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
A cmake_modules/FindKerberosPrograms.cmake
9 files changed, 229 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/9
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 9
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 8: Verified-1 Code-Review+2

> (1 comment)

Manually setting Verified: -1.

Running the test in a loop shows that it's flaky. Not a functional issue, but 
something to do with setting up and tearing down the KDC so often. Or possibly 
a test infra issue because our renew threads don't have a shutdown mechanism.

Will work on fixing it.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 19 Oct 2017 17:35:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 3: Code-Review+2

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 18 Oct 2017 21:20:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 2:

> (1 comment)

I tried cherry-picking:
https://github.com/apache/kudu/commit/50c7d3249ab5ca19fb4d3c0c8748a4a1c5945a12

But since it touches so many files, it doesn't cherry-pick cleanly and adds 
some extra code from patches that we don't have in our code base, due to how 
the diff was generated.

Also, it incorrectly modifies out .clang-tidy file since Kudu's .clang tidy 
does not exist in our fetch of their code. Also, the 3 way merge gets confused 
and ends up modifying our gutil/bind_internal.h instead of their 
kudu/gutil/bind_internal.h.

We can resolve all this manually, but I think it's a bit too risky.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 18 Oct 2017 04:45:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow configuration of values passed into kerberos env vars

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8308 )

Change subject: Allow configuration of values passed into kerberos env vars
..


Patch Set 1:

> Uploaded patch set 1.

This cherry-pick from Kudu was clean and is required for avoiding code 
divergence with Kudu on IMPALA-5129 (https://gerrit.cloudera.org/#/c/7938/)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 18 Oct 2017 00:01:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow configuration of values passed into kerberos env vars

[Sailesh Mukil (Code Review)]

Hello Todd Lipcon,

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

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

to review the following change.


Change subject: Allow configuration of values passed into kerberos env vars
..

Allow configuration of values passed into kerberos env vars

We always used hardcoded constants for the following kerberos
environment variables:

KRB5CCNAME and KRB5RCACHETYPE.

This patch allows for the configuration of these variables by taking
arguments to InitKerberosForServer().

Callsites within Kudu have not been changed as all the parameters have
default values.

The motivation for this patch is that, Impala as a user of the
KuduRPC and Kudu security libraries, needs to have a file based
credential cache since the kinit happens on the C++ side and this cache
needs to be read by the Java side too. Hence, we cannot have it in memory.
Also, Impala still requires replay protection, since some Impala services
use Thrift which lacks the nonce mechanism that KRPC uses for replay
protection.

Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Reviewed-on: http://gerrit.cloudera.org:8080/8247
Reviewed-by: Todd Lipcon 
Tested-by: Todd Lipcon 
---
M be/src/kudu/security/init.cc
M be/src/kudu/security/init.h
2 files changed, 19 insertions(+), 12 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8308/1
--
To view, visit http://gerrit.cloudera.org:8080/8308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iab4ce72c04ec4056dc89fb4c1c540a6fdaca4404
Gerrit-Change-Number: 8308
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

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

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8285 )

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


Patch Set 1:

> Uploaded patch set 1.

This cherry-pick was clean and is intended to fix IMPALA-6058.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8efe68be0ae7f9ab05937d5c81fb53a709a881f1
Gerrit-Change-Number: 8285
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 16 Oct 2017 22:27:07 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

to review the following change.


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
Reviewed-on: http://gerrit.cloudera.org:8080/8273
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M be/src/kudu/rpc/rpcz_store.cc
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/8285/1
--
To view, visit http://gerrit.cloudera.org:8080/8285
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8efe68be0ae7f9ab05937d5c81fb53a709a881f1
Gerrit-Change-Number: 8285
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8272 )

Change subject: IMPALA-6049: breakpad tests: skip all tests with local 
filesystem
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95
Gerrit-Change-Number: 8272
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 13 Oct 2017 16:44:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8270 )

Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8270/1/be/src/testutil/mini-kdc-wrapper.cc
File be/src/testutil/mini-kdc-wrapper.cc:

http://gerrit.cloudera.org:8080/#/c/8270/1/be/src/testutil/mini-kdc-wrapper.cc@17
PS1, Line 17:
I should have done this in the kinit patch, but since this file needs to be 
reused by the rpc-mgr-test, I had to move it to a new file.

I'm not changing the kinit patch, since that's already close to review 
completion, and this is mostly a mechanical move to a new file.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
Gerrit-Change-Number: 8270
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 13 Oct 2017 05:40:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@96
PS7, Line 96: static int kdc_port = GetServerPort();
> static
Done


http://gerrit.cloudera.org:8080/#/c/7938/7/be/src/rpc/thrift-server-test.cc@247
PS7, Line 247: }
> As discussed offline, may be it'd help to test with short renewal period to
This makes the test infra quite flakey. I tried for quite some time, but it 
seems as though starting many KDCs close to each other with different 
configurations seems to be problematic. I'm not sure why yet though.

It is a test infra issue rather than a functional kerberos issue. I think it 
will be better if I took time later to see what the problem could be and add 
the test in a separate patch to save some time.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 13 Oct 2017 05:38:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8270


Change subject: IMPALA-5053: [SECURITY] Make KRPC work with Kerberos
..

IMPALA-5053: [SECURITY] Make KRPC work with Kerberos

KuduRPC has support for Kerberos. However, since Impala's client transport
still uses the Thrift transport stack, we need to make sure that a single
security configuration applies to both internal communication (KuduRPC)
and external communication (Thrift's TSaslTransport).

This patch changes InitAuth() to start Sasl regardless of security
configuration, since KRPC uses plain SASL for negotiation on insecure
clusters.

It also moves some utility code out of authentication.cc into
auth-util.cc for resuse by the RpcMgr while enabling kerberos.

The MiniKDC related code is moved out of thrift-server-test.cc into a
new file called mini-kdc-wrapper.h/cc. This file exposes a new class
MiniKdcWrapper which can be easily used by the tests to configure the
kerberos environment, create the keytab, start the KDC and also
initialize the Impala security library.

Tests are added to rpc-mgr-test for kerberos tests over KRPC.
thrift-server-test also has a mechanical change to use MiniKdcWrapper.
Also tested on a live cluster configured to use kerberos.

Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication-test.cc
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/CMakeLists.txt
A be/src/testutil/mini-kdc-wrapper.cc
A be/src/testutil/mini-kdc-wrapper.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
12 files changed, 415 insertions(+), 169 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/1
--
To view, visit http://gerrit.cloudera.org:8080/8270
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8cec5cca5fdb4b1d46bab19e86cb1a8a3ad718fd
Gerrit-Change-Number: 8270
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig, Dan Hecht,

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

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

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

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..

IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
A cmake_modules/FindKerberosPrograms.cmake
11 files changed, 229 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/8
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8023 )

Change subject: IMPALA-4856: Port data stream service to KRPC
..


Patch Set 3:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/rpc/rpc-mgr.cc@77
PS3, Line 77:
Should we add a log message stating which services we registered with KRPC?

It might be useful later on as we add more services, while trying to debug user 
issues to know which services are on KRPC and which are on thrift. Granted 
there are other ways to find that, but this is easily accessible and 
straightforward.


http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h
File be/src/runtime/krpc-data-stream-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.h@314
PS3, Line 314: w
susper-nit: Capital 'W'


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@198
PS1, Line 198: deseria
> Deserialization pool's purpose is to avoid executing deserialization in lin
Hmm, this seems like it would be a nice thing to have. Is the absence of a 
MemTracker the only hindrance to early deserialization?

Is there some way we could add this to the process MemTracker if we can't 
attribute it to a query? If it's too complicated for now, let's track this with 
a JIRA and write down some ideas there for the next KRPC milestone.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@227
PS1, Line 227: // to make sure that the close operation is performed so add 
to per-recvr list of
 : // pending closes. It's possible for a sender to issue EOS 
RPC without sending any
 : // rows if no rows are m
> This may be a bit subtle but this is equivalent to the logic in the non-KRP
Ah you're right. Case 2 is what changed in IMPALA-5199, but it looks like 
that's automatically fixed here.


http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.cc@112
PS3, Line 112:   for (unique_ptr& ctx : 
early_senders.waiting_sender_ctxs) {
 :  EnqueueRowBatch({recvr->fragment_instance_id(), move(ctx)});
 :  num_senders_waiting_->Increment(-1);
 :   }
 :   for (unique_ptr& ctx : 
early_senders.closed_sender_ctxs) {
 :  recvr->RemoveSender(ctx->request->sender_id());
 :  Status::OK().ToProto(ctx->response->mutable_status());
 :  ctx->context->RespondSuccess();
 :  num_senders_waiting_->Increment(-1);
 :   }
It's not possible for the same sender to be in waiting_senders_ctxs and 
closed_sender_ctxs for a given receiver right?

Because if it would, it would make more sense to service the 
'closed_sender_ctxs' before the 'waiting_sender_ctxs' since we may as well 
close the receiver instead of wasting CPU processing those RPCs for a while.


http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.cc@140
PS3, Line 140:   while (range.first != range.second) {
 : shared_ptr recvr = range.first->second;
 : if (recvr->fragment_instance_id() == finst_id &&
 : recvr->dest_node_id() == node_id) {
 :   return recvr;
 : }
 : ++range.first;
 :   }
I'm thinking it makes sense to prioritize finding the receiver with the 
assumption that we will find it in the receiver_map_, rather than assume that 
it most likely will already be unregistered.

In other words, I think it may be more beneficial CPU-wise for general 
workloads to look in the 'receiver_map_' before looking into the 
'closed_stream_cache_'.

What do you think?


http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.cc@151
PS3, Line 151: KrpcDataStreamMgr::AddEarlySender
We could merge the implementations of AddEarlySender() and 
AddEarlyClosedSender() by using templates and some extra params, but maybe the 
code complexity isn't worth it.


http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.cc@174
PS3, Line 174: // that it is still preparing, so add payload to 
per-receiver list.
Add comment "In the worst case, this RPC is so late that the receiver is 
already unregistered and removed from the closed_stream_cache_, in which case 
it will be responded to by the Maintenance thread after 
FLAGS_datastream_sender_timeout_ms."


http://gerrit.cloudera.org:8080/#/c/8023/3/be/src/runtime/krpc-data-stream-mgr.cc@242
PS3, Line 242:  {
   

[Impala-ASF-CR] IMPALA-6030: Don't start coordinator specific thread pools if a node isn't a coordinator node

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8242 )

Change subject: IMPALA-6030: Don't start coordinator specific thread pools if a 
node isn't a coordinator node
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8242/2/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

http://gerrit.cloudera.org:8080/#/c/8242/2/be/src/runtime/exec-env.h@186
PS2, Line 186: Thread
> I think this is meant to be a thread pool just for ExecQueryFInstances RPC
You're right. Done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1cd046dbe5f10fa51fcc37338e48b94843891b62
Gerrit-Change-Number: 8242
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:20:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6030: Don't start coordinator specific thread pools if a node isn't a coordinator node

[Sailesh Mukil (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-6030: Don't start coordinator specific thread pools if a 
node isn't a coordinator node
..

IMPALA-6030: Don't start coordinator specific thread pools if a node isn't a 
coordinator node

Since we introduced the FLAGS_is_coordinator, we've forgotten to
disable the coordinator specific thread pools on nodes that have
only the executor role.

This patch fixes that.

Change-Id: I1cd046dbe5f10fa51fcc37338e48b94843891b62
---
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
2 files changed, 16 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/8242/3
--
To view, visit http://gerrit.cloudera.org:8080/8242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1cd046dbe5f10fa51fcc37338e48b94843891b62
Gerrit-Change-Number: 8242
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/client_negotiation.cc
File be/src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/client_negotiation.cc@111
PS1, Line 111: const boost::optional& authn_token,
> Should we just take that patch to make (this and) future cherry picks easie
That patch is a tidy cleanup that adds the 'modernize-pass-by-value' rule, 
which we don't use ourselves.

Looking over the patch, it looks like not having it could cause a few more 
conflicts, but they can all be trivially resolved just as I did here.

Also, we will keep running into this issue of cherry-picked patches that 
conflict due to not having the entire history. So it's something we need to 
solve on a case by case basis.

My take is, let's only cherry-pick the patches that we really need. So as for 
this patch, since it doesn't help us a lot, I think we can leave it out.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:12:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6030: Don't start coordinator specific thread pools if a node isn't a coordinator node

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8242 )

Change subject: IMPALA-6030: Don't start coordinator specific thread pools if a 
node isn't a coordinator node
..


Patch Set 2:

> It'd be good to update the header comment for these fields to say
 > that they are set only for coordinator role.

Done.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1cd046dbe5f10fa51fcc37338e48b94843891b62
Gerrit-Change-Number: 8242
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:01:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6030: Don't start coordinator specific thread pools if a node isn't a coordinator node

[Sailesh Mukil (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-6030: Don't start coordinator specific thread pools if a 
node isn't a coordinator node
..

IMPALA-6030: Don't start coordinator specific thread pools if a node isn't a 
coordinator node

Since we introduced the FLAGS_is_coordinator, we've forgotten to
disable the coordinator specific thread pools on nodes that have
only the executor role.

This patch fixes that.

Change-Id: I1cd046dbe5f10fa51fcc37338e48b94843891b62
---
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
2 files changed, 16 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/8242/2
--
To view, visit http://gerrit.cloudera.org:8080/8242
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1cd046dbe5f10fa51fcc37338e48b94843891b62
Gerrit-Change-Number: 8242
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/server_negotiation.cc
File be/src/kudu/rpc/server_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/server_negotiation.cc@128
PS1, Line 128: const security::TokenVerifier* token_verifier,
> Similarly, there was a conflict here since we don't have that same patch fr
Oops, sorry this wasn't a conflict. I misspoke. It was just in 
client_negotiation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 17:55:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 1:

(2 comments)

> Is this a conflict free cherry-pick from something already in Kudu
 > codebase?

Yes this is a cherry-pick from the following kudu commit:
https://github.com/apache/kudu/commit/31d58522b50aea948f977c7cbc8b64f1b849f323

There were a couple of conflicts, but with simple resolutions.

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/client_negotiation.cc
File be/src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/client_negotiation.cc@111
PS1, Line 111: const boost::optional& authn_token,
There was a conflict here since we don't have this patch from the kudu code 
base:
https://github.com/apache/kudu/commit/50c7d3249ab5ca19fb4d3c0c8748a4a1c5945a12#diff-ced439e8f22cae7f007af6cb1daf945a

The above patch changes removes the const ref on this variable.

But the resolution was simple.


http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/server_negotiation.cc
File be/src/kudu/rpc/server_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8230/1/be/src/kudu/rpc/server_negotiation.cc@128
PS1, Line 128: const security::TokenVerifier* token_verifier,
Similarly, there was a conflict here since we don't have that same patch from 
the kudu code base:
https://github.com/apache/kudu/commit/50c7d3249ab5ca19fb4d3c0c8748a4a1c5945a12#diff-ced439e8f22cae7f007af6cb1daf945a

The above patch changes removes the const ref on this variable.

But the resolution was simple.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 17:54:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5940: Avoid log spew by using Status::Expected()

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8255 )

Change subject: IMPALA-5940: Avoid log spew by using Status::Expected()
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8255/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8255/1/be/src/service/impala-server.cc@1170
PS1, Line 1170: const string& err = Substitute("ReportExecStatus(): 
Received report for unknown "
> Actually, we are still printing this line (see line 1172) below. We are jus
Ah ok, got confused. I incorrectly thought it was printing to a higher log 
level that's not enabled by default.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I87493394074a7fdb4e912f4ab2436dd7050d45b3
Gerrit-Change-Number: 8255
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 06:04:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/exec/kudu-util.h@45
PS6, Line 45: (FromKuduStatus(status))
> nit: parenthesis not needed.
Done


http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/auth-provider.h
File be/src/rpc/auth-provider.h:

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/auth-provider.h@144
PS6, Line 144: forks
> nit: forks Impalad and exec 'kinit'
Done


http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@77
PS6, Line 77: "Only used when FLAGS_use_krpc is false");
> Okay. So in other words, we could have fixed the old code as well, and elim
Yes, we could have, but we couldn't deprecate this flag in a minor release 
anyway, so I never went ahead with it.


http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/thrift-server-test.cc@117
PS6, Line 117: !(GetParam() == USE_KUDU_KERBEROS)
> Did you flip the polarity ?
Oops, I did that for some local testing verification and forgot to turn it 
back. Switched it back.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 05:33:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig, Dan Hecht,

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

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

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

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..

IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_kudu_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_kudu_kinit' flag.

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
12 files changed, 230 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/7
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5940: Avoid log spew by using Status::Expected()

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8255 )

Change subject: IMPALA-5940: Avoid log spew by using Status::Expected()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8255/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8255/1/be/src/service/impala-server.cc@1170
PS1, Line 1170: const string& err = Substitute("ReportExecStatus(): 
Received report for unknown "
I find this to be a useful log when debugging user issues. It lets me know if 
there were late RPCs and in some cases if some fragment instances outlived the 
coordinator.

But I agree that this is a lot of noise if printed more than once. Maybe the 
effect of this log will be assuaged by IMPALA-4063?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I87493394074a7fdb4e912f4ab2436dd7050d45b3
Gerrit-Change-Number: 8255
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 11 Oct 2017 05:17:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@77
PS6, Line 77: "Only used when FLAGS_use_krpc is false");
> in what case would a user need to set this flag? and how is that case handl
Currently all our users use this flag. This needs to be set when we use our 
current way of forking kinit (i.e. in other words when FLAGS_use_krpc is false, 
in the context of this patch), since our RunKinit() function uses that to 
periodically wake up.

I'm not sure of the history of this flag, but it is a completely unnecessary 
flag. Our reinit interval should be based on the configured ticket lifetime in 
the system's krb5.conf file, which is what we've done in the kudu kinit case. 
It detects this ticket lifetime and wakes up periodically according to that to 
do the reinit.

Allowing a user to control this flag is very redundant and possibly confusing 
to users that understand kerberos.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:43:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/7938/6/be/src/rpc/authentication.cc@842
PS6, Line 842: if (FLAGS_use_krpc_kinit) {
> can this path only be used with KRPC, or could it be used with thrift?
It can be used with both. The flag is there in case we run into unforeseen 
issues, so we can commit to removing it after one release.

I'll rename it to "use_kudu_kinit", since it's from the kudu security library 
and not the krpc library.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 10 Oct 2017 18:29:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..

IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_krpc_kinit' flag.

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
12 files changed, 230 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/6
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc
File be/src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@460
PS5, Line 460:   //setenv("KRB5CCNAME", "MEMORY:kudu", 1);
 :   //setenv("KRB5_KTNAME", FLAGS_keytab_file.c_str(), 1);
 :   // We commented out the above since we don't want to overwrite 
our own Impala
 :   // set environment variables.
> Is this function used for testing only ? If so, would it be better to move
No, this function is what initializes Kerberos in the kudu security library. 
See L843 in authentication.cc

Unfortunately, there isn't a clean way to make this configurable, unless we 
pass in strings to the InitKerberosForServer() function as parameters for these 
environment variables, which makes little sense for Kudu to have in their code 
base.


http://gerrit.cloudera.org:8080/#/c/7938/5/be/src/kudu/security/init.cc@469
PS5, Line 469:   //setenv("KRB5RCACHETYPE", "none", 1);
> Same comment.
Same as above.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:10:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5789: Add always false flag in bloom filter

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8170 )

Change subject: IMPALA-5789: Add always_false flag in bloom filter
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:

http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator-filter-state.h@57
PS2, Line 57: class Coordinator::FilterState {
Could you add a comment up here explaining the different meanings of 
'always_true' and 'always_false' ?

To me it seems like 'always_false' means either that the FilterState does not 
hold a bloom filter yet, or that the bloom filter is always false.

And 'always_true' could mean that the filter is disabled or that the filter it 
holds is always true.

Also, do these meanings of 'always_true' and 'always_false' hold true in other 
parts of the code as well? i.e. in parts that are not the coordinator?

If it were up to me, I would hold different states for 'disabled_' and 
'is_inited_' instead of trying to derive those meanings implicitly, to make the 
code easier to read. But that's up for debate, and we can reach a consensus 
based on what others say.


http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc@1121
PS2, Line 1121: swap(rpc_params.bloom_filter, aggregated_filter);
What are the possible states after this swap() ? Since we're not explicitly 
setting the disabled case anymore.

Eg: DCHECK(rpc_params.bloom_filter.directory.size() > 0 || 
rpc_params.bloom_filter.always_true == true) ?


http://gerrit.cloudera.org:8080/#/c/8170/2/be/src/runtime/coordinator.cc@1123
PS2, Line 1123: size()
Since you changed the mem tracking from size() to capacity() in L1120 and 
L1181, shouldn't this be capacity() too now?

Else, we risk releasing the capacity() twice, if the swap() doesn't assign a 0 
capacity string to the aggregated filter.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If680240a3cd4583fc97c3192177d86d9567c4f8d
Gerrit-Change-Number: 8170
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:01:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8230 )

Change subject: Allow the SASL protocol service name to be configurable
..


Patch Set 1:

This patch allows Impala to pass in the SASL service name down into the KRPC 
library, so that we can avoid code divergence.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 06 Oct 2017 22:09:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] Allow the SASL protocol service name to be configurable

[Sailesh Mukil (Code Review)]

Hello Dan Burkert,

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

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

to review the following change.


Change subject: Allow the SASL protocol service name to be configurable
..

Allow the SASL protocol service name to be configurable

Previously the SASL service name was always set to a constant "kudu"
which was tracked by kSaslProtoName in rpc/constants.h.

However, for applications that use the KRPC library that would prefer
to do their own SASL initialization, they would requre to set their own
SASL service name to be passed into sasl_server_new()/sasl_client_new().

This patch allows for this configuration by adding a configurable
parameter to the MessengerBuilder which is ultimately passed down to the
negotiation layer.

Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Reviewed-on: http://gerrit.cloudera.org:8080/8218
Reviewed-by: Dan Burkert 
Tested-by: Dan Burkert 
---
M be/src/kudu/rpc/client_negotiation.cc
M be/src/kudu/rpc/client_negotiation.h
M be/src/kudu/rpc/constants.cc
M be/src/kudu/rpc/constants.h
M be/src/kudu/rpc/messenger.cc
M be/src/kudu/rpc/messenger.h
M be/src/kudu/rpc/negotiation-test.cc
M be/src/kudu/rpc/negotiation.cc
M be/src/kudu/rpc/server_negotiation.cc
M be/src/kudu/rpc/server_negotiation.h
10 files changed, 59 insertions(+), 28 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/8230/1
--
To view, visit http://gerrit.cloudera.org:8080/8230
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e30fe4461893b67527333259579e2304b19af1e
Gerrit-Change-Number: 8230
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 

[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads

[Sailesh Mukil (Code Review)]

Sailesh Mukil has abandoned this change. ( http://gerrit.cloudera.org:8080/8094 
)

Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses 
Impala threads
..


Abandoned

Will update with a patch that uses an Impala native ServicePool.
--
To view, visit http://gerrit.cloudera.org:8080/8094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-Change-Number: 8094
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7938 )

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7938/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/7938/4//COMMIT_MSG@21
PS4, Line 21: Converted existing tests in thrift-server-test to run with and
: without kerberos.
> we can also add tests that run with and without FLAGS_use_krpc_kinit set to
Done


http://gerrit.cloudera.org:8080/#/c/7938/4/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/7938/4/be/src/rpc/thrift-server-test.cc@129
PS4, Line 129:   FLAGS_krb5_conf = Substitute("$0/$1", keytab_dir, "krb5.conf");
> since this assert is done a bunch of times here and would probably be done
Good point. We have a FromKuduStatus() function which will come in with 
Michael's RpcMgr patch here:
https://gerrit.cloudera.org/#/c/7901/9/be/src/exec/kudu-util.h

I'll add it for now and it in this patch as well and use it; and it should 
resolve itself in the rebase after his patch is merged in.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 05 Oct 2017 23:27:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork
..

IMPALA-5129: Use KRPC's Kinit code to avoid expensive fork

Impala currently kinits by forking off a child process. This
has proved to be expensive in many cases since the subprocess
tries to reserve as much memory as Impala is currently using
which can be quite a lot.

This patch adds a flag called 'use_krpc_kinit' that defaults to
true. When it's true, it uses the Kudu security library's kinit code
that programatically uses the krb5 library to kinit.
When it's false, we run our current path which kicks off the
kinit-thread and forks off a kinit process periodically to reacquire
tickets based on FLAGS_kerberos_reinit_interval.

Converted existing tests in thrift-server-test to run with and
without kerberos. We now run this BE test with kerberos by using
Kudu's MiniKdc utility. This introduces a new dependency on some
kerberos binaries that are checked through FindKerberosPrograms.cmake.
Note that this is only a test dependency and not a dependency for
the impalad binaries and friends. Compilation will still succeed if
the kerberos binaries for the MiniKdc are not found, however, the
thrift-server-test will fail. We run with and without the
'use_krpc_kinit' flag.

Updated bin/bootstap_development.sh to install new sasl-gssapi
modules and the kerberos binaries required for the MiniKdc.

Testing: Verified with thrift-server-test and also manually on a
live kerberized cluster.

Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/kudu/security/CMakeLists.txt
M be/src/kudu/security/init.cc
M be/src/kudu/security/test/mini_kdc.cc
M be/src/rpc/CMakeLists.txt
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
M be/src/rpc/thrift-server-test.cc
M be/src/testutil/gtest-util.h
M bin/bootstrap_development.sh
A cmake_modules/FindKerberosPrograms.cmake
12 files changed, 234 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/7938/5
--
To view, visit http://gerrit.cloudera.org:8080/7938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cea56cc6e7412d87f4c2e92399a2f91ea6af6c7
Gerrit-Change-Number: 7938
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 14: Verified+1

> Thanks! Also thanks to you and Henry for taking the time to review.
 > (technically my 2nd patch, I removed an invalid DCHECK a few months
 > ago, but it is my first patch of substance)
 > I'll probably start working in my spare time on IMPALA-5393 for my
 > next contribution.
 >
 > > > It looks like it passed gerri-verify-external -
 > https://jenkins.impala.io/job/gerrit-verify-dryrun-external/13/
 > >
 > > Great! I'll rebase and merge this.
 > > Congratulations on getting your first patch in!

Sure thing! Please feel free to reach out at any time. Your commit can be 
viewed here:
https://github.com/apache/incubator-impala/commit/4dd0f1b3d84f67eb40bf671160b057be9bbdb921


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 14
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Thu, 05 Oct 2017 05:41:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] Remove dead code parallel-executor*

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8206 )

Change subject: Remove dead code parallel-executor*
..


Patch Set 1: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8206/1//COMMIT_MSG@7
PS1, Line 7: Remove dead code parallel-executor*
Track it with a JIRA?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f0b121c2c40849937afa103dfedf0e0bef34477
Gerrit-Change-Number: 8206
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:37:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 12: Code-Review+2

> It looks like it passed gerri-verify-external - 
> https://jenkins.impala.io/job/gerrit-verify-dryrun-external/13/

Great! I'll rebase and merge this.
Congratulations on getting your first patch in!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 12
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:23:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 13: Code-Review+2

Rebase, Carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 13
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:23:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8074 )

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
..


Patch Set 2: Code-Review+2

(1 comment)

Rebase, carry +2.

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

http://gerrit.cloudera.org:8080/#/c/8074/1//COMMIT_MSG@12
PS1, Line 12:  in some way used by Impal
> Only gflags in .cc files which aren't dead code will show up, right ? Pleas
Yes, that's right. That's what I found in my experiments. I've updated the 
commit message.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 04 Oct 2017 04:14:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Joe McDonnell,

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

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

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

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
..

IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

Importing Kudu's util/, security/ and rpc/ code into the
Impala code base results in Kudu's gflags showing up in the log
files and the /varz webpages, for all the gflags that belong to
Kudu files that are in some way used by Impala.

For eg: If in Impala, we include kudu/rpc/messenger.h and use one
of its classes whose method definitions live in the corresponding
messenger.cc file, all the flags DEFINEd in the messenger.cc file
will show up in the Impala logs. If we do not use any code in a .cc
file, the flags that are DEFINEd in that file do not show up (this
is probably due to the compiler not running macros in files that
have only dead code).

Since we don't want these flags to be user configurable, we don't
want them to be visible. The following commit earlier included a
gflags patch that added a DEFINE_*_hidden() macro that allows us to
do just this:
https://github.com/apache/incubator-impala/commit/d1910a39fcc50ce211b95c3552c0c90b4bc37bbd

This patch was done by running the following sed command:

find $IMPALA_HOME/be/src/kudu -type f | xargs sed -i 
's/^\(DEFINE_.*\)\((.*\)/\1_hidden\2/g'

There were a couple non-related macros that also got changed:
DEFINE_validator() and DEFINE_STATIC_THREAD_LOCAL(), which were
manually changed back.

Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
---
M be/src/kudu/rpc/acceptor_pool.cc
M be/src/kudu/rpc/messenger.cc
M be/src/kudu/rpc/negotiation-test.cc
M be/src/kudu/rpc/negotiation.cc
M be/src/kudu/rpc/outbound_call.cc
M be/src/kudu/rpc/reactor.cc
M be/src/kudu/rpc/result_tracker.cc
M be/src/kudu/rpc/rpc-bench.cc
M be/src/kudu/rpc/rpc_stub-test.cc
M be/src/kudu/rpc/rpcz_store.cc
M be/src/kudu/rpc/server_negotiation.cc
M be/src/kudu/rpc/service_if.cc
M be/src/kudu/rpc/service_queue-test.cc
M be/src/kudu/rpc/transfer.cc
M be/src/kudu/security/tls_context.cc
M be/src/kudu/security/token_signer.cc
M be/src/kudu/util/cache.cc
M be/src/kudu/util/debug/trace_event_impl.cc
M be/src/kudu/util/env_posix.cc
M be/src/kudu/util/env_util.cc
M be/src/kudu/util/file_cache-stress-test.cc
M be/src/kudu/util/file_cache.cc
M be/src/kudu/util/flag_tags-test.cc
M be/src/kudu/util/flag_validators-test.cc
M be/src/kudu/util/flags-test.cc
M be/src/kudu/util/flags.cc
M be/src/kudu/util/kernel_stack_watchdog.cc
M be/src/kudu/util/logging.cc
M be/src/kudu/util/maintenance_manager.cc
M be/src/kudu/util/memory/arena-test.cc
M be/src/kudu/util/memory/memory.cc
M be/src/kudu/util/metrics.cc
M be/src/kudu/util/minidump.cc
M be/src/kudu/util/mt-hdr_histogram-test.cc
M be/src/kudu/util/mt-metrics-test.cc
M be/src/kudu/util/mutex.cc
M be/src/kudu/util/net/dns_resolver.cc
M be/src/kudu/util/net/net_util.cc
M be/src/kudu/util/net/socket.cc
M be/src/kudu/util/nvm_cache.cc
M be/src/kudu/util/process_memory.cc
M be/src/kudu/util/spinlock_profiling.cc
M be/src/kudu/util/striped64-test.cc
M be/src/kudu/util/test_main.cc
M be/src/kudu/util/test_util.cc
45 files changed, 117 insertions(+), 117 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/74/8074/2
--
To view, visit http://gerrit.cloudera.org:8080/8074
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-Change-Number: 8074
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-5940: Avoid log spew by using Status::Expected.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8100 )

Change subject: IMPALA-5940: Avoid log spew by using Status::Expected.
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h@246
PS3, Line 246: VLOG(1) << err;
> That re-log of the error doesn't include the backtrace though. So I don't t
Yea, the backtrace is not all that useful here. Forgot to add that. I was 
alluding to the fact that from GetSendStatus() we would know that this was a 
RPC failure for the DataStreamSender specifically. Similarly, other clients of 
the RPC subsystem would log this error separately, and we would know what RPC 
failed. So we don't need the backtrace.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330
Gerrit-Change-Number: 8100
Gerrit-PatchSet: 5
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 03 Oct 2017 17:46:10 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5940: Avoid log spew by using Status::Expected.

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8100 )

Change subject: IMPALA-5940: Avoid log spew by using Status::Expected.
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h@246
PS3, Line 246: VLOG(1) << err;
> Michael/Sailesh - are you guys okay with eliminating this backtrace, or hav
I think it should be fine, since we re-log the error here anyway:
https://github.com/apache/incubator-impala/blob/e993b9712c81dfb66fdf65bb5269cdc38a8eef18/be/src/runtime/data-stream-sender.cc#L278

Same for RetryRpcRecv() as well.

Michael can confirm if it's okay too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330
Gerrit-Change-Number: 8100
Gerrit-PatchSet: 5
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 03 Oct 2017 17:23:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 10:

(1 comment)

Thanks Mike. @John: I think a rebase and a fix of the default args should 
suffice before kicking off another run.

http://gerrit.cloudera.org:8080/#/c/7061/10/be/src/rpc/TAcceptQueueServer.h
File be/src/rpc/TAcceptQueueServer.h:

http://gerrit.cloudera.org:8080/#/c/7061/10/be/src/rpc/TAcceptQueueServer.h@135
PS10, Line 135: int32_t maxTasks = 0
> Here and elsewhere, clang-tidy and clang compilations are unhappy with the
Yes, you would need to move the default arguments to the declarations instead 
of having them in the definitions here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 10
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 03 Oct 2017 17:11:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 9:

(1 comment)

Yes, a run of the gerrit-verify-dryrun-external is a good idea. If that passes, 
we can +2 this patch and merge it.

I just have one nit review about a code comment otherwise.

http://gerrit.cloudera.org:8080/#/c/7061/9/be/src/transport/TSaslServerTransport.cpp
File be/src/transport/TSaslServerTransport.cpp:

http://gerrit.cloudera.org:8080/#/c/7061/9/be/src/transport/TSaslServerTransport.cpp@162
PS9, Line 162: // It would be possible for the transport to be created 
multiple times if this method
 : // was called concurrently on the same 'trans' object. The 
current usage of this
 : // method is that it is always called serially.
Instead of saying it's always called serially, I think this comment is clearer:

"This method should never be called concurrently with the same 'trans' object. 
Therefore, it is safe to drop the transportMap_mutex_ here."



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 9
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 03 Oct 2017 00:09:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 6: Code-Review+2

Hit IMPALA-5999 twice in a row. Trying GVO again.

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:16:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 8:

> No problem, I just got back from a month long wedding/honeymoon
 > myself.
 >
 > I'll try to address your comments in the next few days.
 >
 > > (3 comments)
 > >
 > > Sorry for the slow response and thanks for your patience, John.
 > > Henry is away on holiday, but let's try to get this in in the
 > > meantime. The patch looks good to me. I just have a couple small
 > > comments. And rebasing the patch before posting the next patch
 > set
 > > would be a good idea.

Sounds good. And congratulations!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 8
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:52:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 8:

(3 comments)

Sorry for the slow response and thanks for your patience, John. Henry is away 
on holiday, but let's try to get this in in the meantime. The patch looks good 
to me. I just have a couple small comments. And rebasing the patch before 
posting the next patch set would be a good idea.

http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc@403
PS8, Line 403: TEST(ConcurrencyTest, MaxConcurrentConnections) {
Could you write a brief comment about what this test is trying to achieve?


http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc@439
PS8, Line 439:   EXPECT_TRUE(did_reach_max);
> Not sure if this will be effective. Could end up being racy? If so probably
It looks like it could be racy, but very rarely. We have no control over how 
the OS would schedule these threads. We can revisit this test later if it shows 
up to be a problem, but for now, I'm okay with it.


http://gerrit.cloudera.org:8080/#/c/7061/3/be/src/transport/TSaslServerTransport.cpp
File be/src/transport/TSaslServerTransport.cpp:

http://gerrit.cloudera.org:8080/#/c/7061/3/be/src/transport/TSaslServerTransport.cpp@164
PS3, Line 164:   new TSaslServerTransport(serverDefinitionMap_, trans));
It would be good to add the comment here acknowledging the "logical" race, i.e. 
we know a logical race exists but it won't ever happen because getTransport() 
won't be called concurrently from different threads with the same 'trans' 
object.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 8
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:45:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5: Code-Review+2

Hit a flaky test. Will investigate and open a JIRA.

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:13:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 4: Code-Review+2

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:33:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8023 )

Change subject: IMPALA-4856: Port data stream service to KRPC
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@198
PS1, Line 198: AddData
Isn't the point of the deserialize pool to deserialize the payload early?
Here, we're just calling AddData() on the payloads for early senders after the 
corresponding receiver has been created.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@165
PS1, Line 165: Either we'll consume a row batch from batch_queue_, or it's empty
Shouldn't there always be something in the batch_queue_ if there's something in 
the blocked_senders_ list? Since we fill the blocked_senders_ only if the queue 
is at its limit.

And we also logically service the batches from batch_queue_ first before 
servicing the batches from the blocked_senders_ list.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@166
PS1, Line 166: There is a window
Just to make things clearer, could you specify what there's a window for?


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@225
PS1, Line 225:
There is a problem here. When we release lock_here, an arbitrary number of 
senders could call AddBatch(), and all their batches would get enqueued even 
though the ExceedsLimit() would be true. This breaks the guarantee of the queue 
not being over committed more than a single batch.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@284
PS1, Line 284:   for (const auto& queue_entry: batch_queue_) delete 
queue_entry.second;
batch_queue_.clear() ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1
Gerrit-Change-Number: 8023
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 26 Sep 2017 18:11:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.h@999
PS2, Line 999:   /// Init() were <= 0.
> add to comment:
Done


http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.cc@2078
PS2, Line 2078:   }
> i think we still need to reset these pointers since they hold a shared_ptr
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 26 Sep 2017 14:38:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..

IMPALA-4786: Clean up how ImpalaServers are created

ImpalaServer had to be created via an awkward CreateImpalaServer()
method that took a lot of arguments. This patch refactors that code (in
anticipation of some KRPC changes) to follow a more normal lifecycle:

1. ImpalaServer* server = new ImpalaServer(ExecEnv*)
2. RETURN_IF_ERROR(server->Init()) // for error-returning init operations
3. RETURN_IF_ERROR(server->Start())
4. server->Join()

Also add ExecEnv::Init(), and move calls to ExecEnv::StartServices() to
ImpalaServer::StartServices(). This captures a dependency that KRPC will
rely on - where initialization of both ExecEnv and ImpalaServer need to
happen before services are started.

This sets up a clean-up of InProcessImpalaServer, which is too heavy for
the work that it does. That work is deferred to a follow-on patch.

This is a slightly cleaned up version of Henry's abandoned patch:
https://gerrit.cloudera.org/#/c/7673/

Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
---
M be/src/exprs/expr-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/impalad-main.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/hdfs-util-test.cc
10 files changed, 128 insertions(+), 137 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8076/3
--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8023 )

Change subject: IMPALA-4856: Port data stream service to KRPC
..


Patch Set 1:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h
File be/src/runtime/krpc-data-stream-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h@110
PS1, Line 110: sent
nit: received


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h@110
PS1, Line 110: no TransmitData() RPCs will successfully deliver their
 : /// payload.
Why would there be a TransmitData() RPC if EndDataStream() has already been 
sent? Doesn't the sender send it only if it knows all its TransmitData() RPCs 
have been processed?


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h@133
PS1, Line 133: /// period expires.
As per Tim's comment above, I would also reference IMPALA-3990 as a TODO here 
for later fixing.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h@159
PS1, Line 159: Consider tracking, on the sender, whether a batch has been 
successfully sent or
 : ///   not. That's enough state to realise that a receiver has 
failed (rather than not
 : ///   prepared yet), and the data stream mgr can use that to 
fail an RPC fast, rather than
 : ///   having the closed-stream list.
It would be nice to have a JIRA for this and reference it here.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h@353
PS1, Line 353: waiting_senders
This is a little confusing to follow in the .cc file, since when I see 
"waiting_senders", I expect it to be a set of some unique identifiers for a 
Sender ID.

Although this is unique to a specific sender, it would be a little clearer to 
call this 'waiting_senders_ctxs'.

Let me know what you think.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.h@356
PS1, Line 356: closed_senders
Similarly, we could call this 'closed_senders_ctxs'.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@168
PS1, Line 168:   num_senders_waiting_->Increment(1);
 :   total_senders_waited_->Increment(1);
 :   RecvrId recvr_id = make_pair(fragment_instance_id, 
request->dest_node_id());
 :   auto payload =
 :   make_unique(proto_batch, context, 
request, response);
 :   
early_senders_map_[recvr_id].waiting_senders.push_back(move(payload));
I'm wondering if it makes sense to add simple inline functions that encapsulate 
this functionality; for the sake of readability.

Eg: AddEarlyWaitingSender(), AddEarlyClosedSender()


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@213
PS1, Line 213: If no receiver found, but not in the closed stream cache
nit: If no receiver is found, and the receiver is not in the closed stream 
cache as well, we still need...


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@218
PS1, Line 218:   RecvrId recvr_id = make_pair(fragment_instance_id, 
dest_node_id);
 :   auto payload = make_unique(context, 
request, response);
 :   
early_senders_map_[recvr_id].closed_senders.emplace_back(move(payload));
 :   num_senders_waiting_->Increment(1);
 :   total_senders_waited_->Increment(1);
AddEarlyClosedSender() as per comment above, if you agree.


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-mgr.cc@227
PS1, Line 227:   if (LIKELY(recvr != nullptr)) 
recvr->RemoveSender(request->sender_id());
 :   Status::OK().ToProto(response->mutable_status());
 :   context->RespondSuccess();
This may need some modification based on the recent commit for IMPALA-5199:
https://github.com/apache/incubator-impala/commit/5119ced50c0e0c4001621c9d4da598c187bdb580


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc
File be/src/runtime/krpc-data-stream-recvr.cc:

http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@75
PS1, Line 75: ("new data")
I'm having some trouble understanding what this means. Could you please clarify?


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@271
PS1, Line 271: data_arrival_cv_.notify_all();
Shouldn't this notify be done while holding the lock_ ?


http://gerrit.cloudera.org:8080/#/c/8023/1/be/src/runtime/krpc-data-stream-recvr.cc@285
PS1, Line 285:   while (!blocked_senders_.empty()) {
nit: Add comment: Respond to blocked senders' RPCs

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..


Patch Set 10: Code-Review+1

I went over the fix to AddTimeSeriesCounter, and it looks correct. This patch 
LGTM.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..


Patch Set 9: Code-Review+1

(2 comments)

LGTM, except these 2 comments.

http://gerrit.cloudera.org:8080/#/c/8069/9/be/src/exec/data-source-scan-node.cc
File be/src/exec/data-source-scan-node.cc:

PS9, Line 368:   
PeriodicCounterUpdater::StopRateCounter(total_throughput_counter());
 :   
PeriodicCounterUpdater::StopTimeSeriesCounter(bytes_read_timeseries_counter_);
Spurious. These should be stopped in ScanNode::Close() ?


http://gerrit.cloudera.org:8080/#/c/8069/9/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

PS9, Line 244: PeriodicCounterUpdater::StopTimeSeriesCounter(
 :   recvr_->bytes_received_time_series_counter_);
Seems like an outlier.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..


Patch Set 8:

(3 comments)

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

Line 19: 
Could you document that you removed the time_series_counter_map_lock_ and the 
reasoning?


http://gerrit.cloudera.org:8080/#/c/8069/8/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

PS8, Line 178: profile_
How come we don't stop counters for this profile? There are other profiles in 
other files where we don't stop them too.

Wouldn't that be a slight change in behavior? Since they were previously 
stopped in the destructor?


http://gerrit.cloudera.org:8080/#/c/8069/8/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS8, Line 950: vector* 
RuntimeProfile::AddBucketingCounters(Counter* src_counter,
Long line


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8069/7/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS7, Line 85: DCHECK(!has_active_periodic_counters_);
#ifndef NDEBUG
lock_guard l(counter_map_lock_);
DCHECK(!has_active_periodic_counters_);
#endif

For correctness?


PS7, Line 88: void RuntimeProfile::StopPeriodicCounters() {
This doesn't stop the counters that belong to child profiles. Is it on the user 
to stop counters of child profiles?


http://gerrit.cloudera.org:8080/#/c/8069/7/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

PS7, Line 407: rate_counters
nit: rate_counters_


PS7, Line 408: sampling_counters
nit: sampling_counters_


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1952: 
> what is the purpose of this? I think we should avoid shared_from_this() unl
Since the Thrift classes expect a boost::shared_ptr, we need to pass in a 
boost::shared_ptr to the interface of 
ImpalaHiveServer2ServiceProcessor, etc.

shared_from_this() is basically a shared_ptr version of the 'this' pointer. So 
this looks like the cleanest way to do this.


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 123: thrift_
> is that the thrift impala internal service port (as opposed to to krpc veri
It's the thrift internal service port. I renamed it to thrift_be_port.


Line 999:   /// Init() were <= 0.
> can you comment on who owns these? even better, if this class owns them, us
Changed to scoped_ptr.


PS1, Line 1002: ptr since krpc is on the way, how about thrift_be_server_? this goes away once 
Yes, that's right. I've renamed it.


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

PS1, Line 86: boost::shared_ptr impala_server
> why do we need shared ownership of this thing? who else needs to keep it al
Yes, they are a consequence of the Thrift interface expecting a 
boost::shared_ptr.

The Thrift classes are:
ImpalaServiceProcessor, ImpalaHiveServer2ServiceProcessor, etc.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes