[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..


Patch Set 6: Code-Review+2

Carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
Gerrit-Change-Number: 9060
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 22 Jan 2018 22:25:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Burkert, Philip Zeyliger,

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

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

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

Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..

IMPALA-6418: Find a reliable way to detect supported TLS versions

Due to a RHEL bug, finding the version of the dynamically linked
OpenSSL is unreliable on certain RHEL platforms. This means that
if Impala is built against OpenSSL 1.0.0 and run against OpenSSL
1.0.1, due to the bug, Impala will not be able to access the
capabilities of OpenSSL 1.0.1. The most significant drawback of
this is that in this scenario, Impala cannot use TLSv1.2, even though
it is available.

The RHEL bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1497859

This patch imitates Kudu's way of determining what TLS versions are
supported via this patch:
https://github.com/apache/kudu/commit/b88117415a02699c12a6eacbf065c4140ee0963c

This is tested and works on all supported platforms of Impala.

Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
4 files changed, 45 insertions(+), 12 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
Gerrit-Change-Number: 9060
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Burkert, Philip Zeyliger,

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

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

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

Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..

IMPALA-6418: Find a reliable way to detect supported TLS versions

Due to a RHEL bug, finding the version of the dynamically linked
OpenSSL is unreliable on certain RHEL platforms. This means that
if Impala is built against OpenSSL 1.0.0 and run against OpenSSL
1.0.1, due to the bug, Impala will not be able to access the
capabilities of OpenSSL 1.0.1. The most significant drawback of
this is that in this scenario, Impala cannot use TLSv1.2, even though
it is available.

The RHEL bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1497859

This patch imitates Kudu's way of determining what TLS versions are
supported via this patch:
https://github.com/apache/kudu/commit/b88117415a02699c12a6eacbf065c4140ee0963c

This is tested and works on all supported platforms of Impala.

Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
4 files changed, 45 insertions(+), 12 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
Gerrit-Change-Number: 9060
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Burkert, Philip Zeyliger,

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

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

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

Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..

IMPALA-6418: Find a reliable way to detect supported TLS versions

Due to a RHEL bug, finding the version of the dynamically linked
OpenSSL is unreliable on certain RHEL platforms. This means that
if Impala is built against OpenSSL 1.0.0 and run against OpenSSL
1.0.1, due to the bug, Impala will not be able to access the
capabilities of OpenSSL 1.0.1. The most significant drawback of
this is that in this scenario, Impala cannot use TLSv1.2, even though
it is available.

The RHEL bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1497859

This patch imitates Kudu's way of determining what TLS versions are
supported via this patch:
https://github.com/apache/kudu/commit/b88117415a02699c12a6eacbf065c4140ee0963c

This is tested and works on all supported platforms of Impala.

Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
4 files changed, 43 insertions(+), 10 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
Gerrit-Change-Number: 9060
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Burkert, Philip Zeyliger,

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

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

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

Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..

IMPALA-6418: Find a reliable way to detect supported TLS versions

Due to a RHEL bug, finding the version of the dynamically linked
OpenSSL is unreliable on certain RHEL platforms. This means that
if Impala is built against OpenSSL 1.0.0 and run against OpenSSL
1.0.1, due to the bug, Impala will not be able to access the
capabilities of OpenSSL 1.0.1. The most significant drawback of
this is that in this scenario, Impala cannot use TLSv1.2, even though
it is available.

The RHEL bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1497859

This patch imitates Kudu's way of determining what TLS versions are
supported via this patch:
https://github.com/apache/kudu/commit/b88117415a02699c12a6eacbf065c4140ee0963c

This is tested and works on all supported platforms of Impala.

Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
4 files changed, 38 insertions(+), 10 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
Gerrit-Change-Number: 9060
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6338: Fix flaky test profile fragment instances

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@206
PS2, Line 206: either because the client called Cancel() or because the 
coordinator
 :   /// initiated cancel because all the results were already 
returned
It could also be because another backend hit an error.

Do we know of any other ways cancellation can be initiated? See more in my 
comment below.


http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@243
PS2, Line 243: other than cancel
We're making a base assumption that a CANCELLED status is only obtained as a 
result of the Coordinator setting and propagating it (for any reason).

But are we sure that's true (It would be ideal if that were true)? i.e. are we 
sure that there's no fragment instance cancelling itself (and hence the query) 
for some other reason?

Because if that was the case, then with this patch, a fragment instance trying 
to cancel the query wouldn't cause the query to be cancelled. Since the 
coordinator is assuming that if it sees a CANCELLED status from a fragment 
instance, that's due to the coordinator setting it.


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

http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc@926
PS2, Line 926: // If the query was cancelled by the user, don't process the 
update.
 :   if (query_status_.IsCancelled()) return Status::OK();
Shouldn't we tell the backend to cancel itself if the query was cancelled by 
the user? Or are we relying on the Cancel() RPCs doing that?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a1e3c62952003f37f88fe2b662bb11889ed
Gerrit-Change-Number: 8997
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sat, 20 Jan 2018 00:45:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6399: Fix timeout logic in test query profile thrift timestamps

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6399: Fix timeout logic in 
test_query_profile_thrift_timestamps
..


Patch Set 2: Code-Review+2

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/9079/2//COMMIT_MSG@10
PS2, Line 10: there was a bug in the retry logic
Could you also briefly add what this bug was in the commit message?


http://gerrit.cloudera.org:8080/#/c/9079/2//COMMIT_MSG@15
PS2, Line 15: to forget the it
typo


http://gerrit.cloudera.org:8080/#/c/9079/2/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/9079/2/tests/query_test/test_observability.py@151
PS2, Line 151: introduce an error below
I don't think that it's important to state this line. But if you feel like it's 
better to keep it, I have a nit:
"...introduce an error below in future changes..."



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0f3db75c52b8b2081dd15ed7e7d2a1d6b22389
Gerrit-Change-Number: 9079
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 19 Jan 2018 22:38:56 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 4:

(15 comments)

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

http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@78
PS4, Line 78: string IMPALA_HOME(getenv("IMPALA_HOME"));
> const static
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@90
PS4, Line 90: TLSv1.0 compatible ciphers
> Are there ways to detect the TLS version supported on the platform and choo
There actually is a way now, but the patch that introduces some basic utils for 
it is still in review here:
https://gerrit.cloudera.org/#/c/9060/

I can add tests for TLSv1.2 as a follow on patch.


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@268
PS4, Line 268: RunTlsTestTemplate
> What do you think about factoring line 331 to line 371 below into a single
Done. I removed RunTlsTestTemplate() and made every test use the new function 
RunMultipleServicesTestTemplate()


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@309
PS4, Line 309:
> nit: blank line
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@331
PS4, Line 331:   // Test that a service can be started, and will respond to 
requests.
 :   unique_ptr ping_impl(
 :   new PingServiceImpl(tls_rpc_mgr.metric_entity(), 
tls_rpc_mgr.result_tracker()));
 :   ASSERT_OK(tls_rpc_mgr.RegisterService(10, 10, 
move(ping_impl)));
 :
 :   // Test that a second service, that verifies the RPC payload 
is not corrupted,
 :   // can be started.
 :   unique_ptr scan_mem_impl(
 :   new ScanMemServiceImpl(tls_rpc_mgr.metric_entity(), 
tls_rpc_mgr.result_tracker()));
 :   ASSERT_OK(tls_rpc_mgr.RegisterService(10, 10, 
move(scan_mem_impl)));
 :
 :   FLAGS_num_acceptor_threads = 2;
 :   FLAGS_num_reactor_threads = 10;
 :   ASSERT_OK(tls_rpc_mgr.StartServices(tls_krpc_address));
 :
 :   unique_ptr ping_proxy;
 :   
ASSERT_OK(tls_rpc_mgr.GetProxy(tls_krpc_address, 
_proxy));
 :
 :   unique_ptr scan_mem_proxy;
 :   
ASSERT_OK(tls_rpc_mgr.GetProxy(tls_krpc_address, 
_mem_proxy));
 :
 :   RpcController controller;
 :   srand(0);
 :   // Randomly invoke either services to make sure a RpcMgr can 
host multiple
 :   // services at the same time.
 :   for (int i = 0; i < 100; ++i) {
 : controller.Reset();
 : if (random() % 2 == 0) {
 :   PingRequestPB request;
 :   PingResponsePB response;
 :   kudu::Status status = ping_proxy->Ping(request, , 
);
 :   ASSERT_TRUE(status.ok());
 :   ASSERT_EQ(response.int_response(), 42);
 : } else {
 :   ScanMemRequestPB request;
 :   ScanMemResponsePB response;
 :   SetupScanMemRequest(, );
 :   kudu::Status status = scan_mem_proxy->ScanMem(request, 
, );
 :   ASSERT_TRUE(status.ok());
 : }
 :   }
> Please see comments above on how this may be factored into a function to be
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@378
PS4, Line 378:   // Misconfigure TLS with bad CA certificate.
> nit: may as well move it to before line 377 as block comment to describe wh
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@392
PS4, Line 392:
 :   // Misconfigure TLS with a bad password command for the 
passowrd protected private key.
> nit: may as well move it to before line 391 as block comment for this funct
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@412
PS4, Line 412:   // Configure TLS with a password protected private key and the 
correct password for it.
> nit: same here.
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@430
PS4, Line 430:   // Misconfigure TLS with a bad cipher.
> nit: same here
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@446
PS4, Line 446:   // Enable TLS with a TLSv1 compatible cipher list.
> nit: same here.
Done


http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.h@100
PS4, Line 100: {
 :   }
> nit: one line { }
Done


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

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

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, 329 insertions(+), 68 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8439/5
--
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: newpatchset
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5528: Bump total thread cache size when KRPC is enabled

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5528: Bump total thread cache size when KRPC is enabled
..


Patch Set 1:

(1 comment)

Are the corresponding test changes to buffer-allocator-test, free-list-test and 
suballocator-test not required?

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

http://gerrit.cloudera.org:8080/#/c/9058/1//COMMIT_MSG@30
PS1, Line 30: TCMALLOC_TRANSFER_NUM_OBJ will be tuned in a separate change. 
Previous attempt
Nit: GPerfTools/TCMalloc needs to be upgraded to 2.6.3 to pick up the
change of the default value of TCMALLOC_TRANSFER_NUM_OBJ.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8407528942051fb19a0222491347c9090d4b4b8d
Gerrit-Change-Number: 9058
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 18 Jan 2018 22:06:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..


Patch Set 4: Code-Review+2

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
Gerrit-Change-Number: 9006
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 18 Jan 2018 21:42:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker,

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

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

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

Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..

IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

In KrpcDataStreamMgr::CreateRecvr() we take the lock_ and
then call recvr->TakeOverEarlySender() for all contexts.
recvr->TakeOverEarlySender() then calls
recvr_->mgr_->EnqueueDeserializeTask((), which can block if the
deserialize pool queue is full. The next thread to become available
in that queue will also have to acquire lock_, thus leading to a
deadlock.

We fix this by moving the EarlySendersList out of the
EarlySendersMap and dropping the lock before taking any actions on
the RPC contexts in the EarlySendersList. All functions called after
dropping 'lock_' do not require the lock to protect them as they are
thread safe.

Additionally modified the BE test data-stream-test to work with KRPC
as well.

Testing: Added a new test to data-stream-test to verify that the
deadlock does not happen. Also, I verified that this test hangs
without the fix.

Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
2 files changed, 256 insertions(+), 57 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
Gerrit-Change-Number: 8950
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@689
PS3, Line 689: ::testing::Values(USE_KRPC));
 :
 : TEST_P(DataStreamTest, UnknownSenderSmallResult) {
 :   // starting a sender w/o a corresponding receiver results in 
an error. No bytes should
 :   // be sent.
 :   // case 1: entire query result fits in single buffer
 :   TUniqueId dummy_id;
 :   GetNextInstanceId(_id);
 :   StartSender(TPartitionType::UNPARTITIONED, TOTAL_DATA_SIZE + 
1024);
 :
> OK. I see. So, it's pattern matching against the class name to pass the rig
We can do that, but then the test output shows that the test has run 
successfully against the option that we wanted to skip.

I'm okay with doing that too, but I just thought that this would be clearer to 
read from the output.


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@703
PS3, Line 703: TEST_P(DataStreamTest, UnknownSenderLargeResult) {
 :   // case 2: query result requires multiple buffers
> Are we allowed to run with more than one switches ? I wonder if it makes se
We could iterate through different config values like the link above and run 
tests, but that would mean we would need to run all the tests through the 
different config values, which is unnecessary.

The other reason for specializing this class is to run it only with KRPC.


http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@130
PS4, Line 130: };
> Brief comments about what this class is for.
Done


http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@282
PS4, Line 282: scoped_ptr
> // Only used for KRPC. Not owned.
Done


http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@283
PS4, Line 283: _val_;
> Should this be initialized to nullptr in ctor or here ? Same for stream_mgr
I set them both to nullptr here.


http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@550
PS4, Line 550:
> nit: extra blank line
Done


http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@799
PS4, Line 799: // the senders' payloads reach the receiver before the receiver 
is setup. Once the
> It'd be nice to add a brief description of this test to explain how it exer
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
Gerrit-Change-Number: 8950
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 18 Jan 2018 21:40:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..


Patch Set 1:

(1 comment)

> Is there a convenient place where we can log the relevant
 > MaxSSLVersion() at startup, perhaps only where we want to use SSL?

The MaxTlsVersionSupported() returns an internal representation of the version 
rather than something like TLSv1.2. So we could log it, but it wouldn't make 
too much sense to the user. I'm also against adding our own conversion layer 
from their internal representation to a human readable string, as that could 
easily change between versions.

http://gerrit.cloudera.org:8080/#/c/9060/1/be/src/util/openssl-util.h
File be/src/util/openssl-util.h:

http://gerrit.cloudera.org:8080/#/c/9060/1/be/src/util/openssl-util.h@90
PS1, Line 90: mode_ = MaxSupportedTlsVersion() < TLS1_2_VERSION ? 
AES_256_CFB : AES_256_CTR;
> Looks like this was already the case, but won't this prohibit running a sin
We don't explicitly support that configuration, i.e. different OpenSSL versions 
on different nodes.

Also, this part of the code is currently only used for spill-to-disk 
operations, which is node local.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
Gerrit-Change-Number: 9060
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 18 Jan 2018 21:12:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6418: Find a reliable way to detect supported TLS versions

[Sailesh Mukil (Code Review)]

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


Change subject: IMPALA-6418: Find a reliable way to detect supported TLS 
versions
..

IMPALA-6418: Find a reliable way to detect supported TLS versions

Due to a RHEL bug, finding the version of the dynamically linked
OpenSSL is unreliable on certain RHEL platforms. This means that
if Impala is built against OpenSSL 1.0.0 and run against OpenSSL
1.0.1, due to the bug, Impala will not be able to access the
capabilities of OpenSSL 1.0.1. The most significant drawback of
this is that in this scenario, Impala cannot use TLSv1.2, even though
it is available.

The RHEL bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1497859

This patch imitates Kudu's way of determining what TLS versions are
supported via this patch:
https://github.com/apache/kudu/commit/b88117415a02699c12a6eacbf065c4140ee0963c

This is tested and works on all supported platforms of Impala.

Change-Id: Idd40219b7be5889b3c24457acdb79a28bdcd9bfb
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
4 files changed, 36 insertions(+), 9 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-6395: Add a flag for data stream sender's buffer size

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6395: Add a flag for data stream sender's buffer size
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I385f4b7a0671bb2d7872bee60d476c375680b5c2
Gerrit-Change-Number: 9026
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 18 Jan 2018 18:49:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker,

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

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

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

Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..

IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

In KrpcDataStreamMgr::CreateRecvr() we take the lock_ and
then call recvr->TakeOverEarlySender() for all contexts.
recvr->TakeOverEarlySender() then calls
recvr_->mgr_->EnqueueDeserializeTask((), which can block if the
deserialize pool queue is full. The next thread to become available
in that queue will also have to acquire lock_, thus leading to a
deadlock.

We fix this by moving the EarlySendersList out of the
EarlySendersMap and dropping the lock before taking any actions on
the RPC contexts in the EarlySendersList. All functions called after
dropping 'lock_' do not require the lock to protect them as they are
thread safe.

Additionally modified the BE test data-stream-test to work with KRPC
as well.

Testing: Added a new test to data-stream-test to verify that the
deadlock does not happen. Also, I verified that this test hangs
without the fix.

Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
2 files changed, 240 insertions(+), 56 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
Gerrit-Change-Number: 8950
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@138
PS3, Line 138: TransmitDataResponsePB* response, RpcContext* rpc_context) {
> nit: indent off.
Done


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@143
PS3, Line 143: EndDataStreamResponsePB* response, RpcContext* rpc_context) {
> nit: indent off.
Done


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@174
PS3, Line 174: if (GetParam() == USE_THRIFT) {
 :   FLAGS_use_krpc = false;
 : } else {
 :   FLAGS_use_krpc = true;
 : }
> FLAGS_use_krpc = GetParam() == USE_KRPC;
Done


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@194
PS3, Line 194: stream_mgr_ = ExecEnv::GetInstance()->stream_mgr();
> Can't this line be factored out ?
Done


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@557
PS3, Line 557:  FLAGS_datastream_service_num_svc_threads
> Do we actually customize this flag ? if not, seems simpler to just start so
No we don't. Done.


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@560
PS3, Line 560: FLAGS_datastream_service_queue_depth
> Same comment about this flag.
Done


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@583
PS3, Line 583: info.thread_handle
> nit: indent off.
Done


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@601
PS3, Line 601: void ThriftSender
> This seems to be mostly identical to KrpcSender. Can we refactor the code t
Yes it can be done by making the sender object as a DataSink type and casting 
where necessary. Done.


http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@689
PS3, Line 689: class DataStreamTestForImpala2931 : public DataStreamTest {
 :  protected:
 :   virtual void SetUp() {
 : DataStreamTest::SetUp();
 :   }
 :
 :   virtual void TearDown() {
 : DataStreamTest::TearDown();
 :   }
 : };
> What's special about this class ? Isn't it mostly the same as DataStreamtes
It is, but we only want to run this test for Thrift and not for KRPC. So if you 
look below in L716, we only pass in the USE_THRIFT flag for 
DataStreamTestForImpala2931.


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

http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/krpc-data-stream-mgr.cc@113
PS3, Line 113:   // Move the early senders list here so that we can drop 
'lock_'.
> Please also add a comment that we must drop the lock_ before processing the
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
Gerrit-Change-Number: 8950
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 16 Jan 2018 23:25:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9006/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9006/3/CMakeLists.txt@310
PS3, Line 310: Tests
> What would happen before we switched over to using Kudu library for Kerbero
This bug existed even before we started using the kudu security library. So far 
we haven't had any users complain about this. Keep in mind that this is a 
kerberos bug and we're just working around it for our tests. My take is that we 
can leave the old code as it is since it isn't particularly biting anyone.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
Gerrit-Change-Number: 9006
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 16 Jan 2018 22:24:08 +
Gerrit-HasComments: Yes

[native-toolchain-CR] IMPALA-6377: Bump breakpad version to include fix for #752

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6377: Bump breakpad version to include fix for #752
..


Patch Set 1: Code-Review+2

(3 comments)

Just a few nits.

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

http://gerrit.cloudera.org:8080/#/c/9036/1//COMMIT_MSG@7
PS1, Line 7: #752
Could you add a link for this?


http://gerrit.cloudera.org:8080/#/c/9036/1//COMMIT_MSG@8
PS1, Line 8:
Not sure if you do this on every version bump, but it would be good to include 
a link to the JIRA regarding the DWZ patch, and just say that you're 
re-applying it for the new version.


http://gerrit.cloudera.org:8080/#/c/9036/1//COMMIT_MSG@9
PS1, Line 9: This also removes the PPC patches from Breakpad, which I was not 
able to
   : get to apply cleanly.
Add that the PPC patch will get re-added in a later commit, after the patch is 
fixed for the latest version.



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b6212ab77eff2df0cb51339c25183ae4f22b07b
Gerrit-Change-Number: 9036
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 16 Jan 2018 21:54:56 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 4:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/8439/3/be/src/rpc/rpc-mgr.h@105
PS3, Line 105:
> Seems more appropriate to pass it to the constructor of RpcMgr() instead of
Done


http://gerrit.cloudera.org:8080/#/c/8439/3/be/src/rpc/rpc-mgr.h@182
PS3, Line 182:
> True if TLS is configured for communication between Impalad backend servers
Done


http://gerrit.cloudera.org:8080/#/c/8439/3/be/src/rpc/rpc-mgr.h@183
PS3, Line 183: /// True if TLS is con
> const bool use_tls_; and initialize it in the ctor.
Done


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

http://gerrit.cloudera.org:8080/#/c/8439/3/be/src/rpc/rpc-mgr.cc@88
PS3, Line 88: bld.set_rpc_authentication("required");
:   }
> This may be worth fixing too in this patch if it's not too complicated.
Done


http://gerrit.cloudera.org:8080/#/c/8439/3/be/src/rpc/rpc-mgr.cc@117
PS3, Line 117:   RETURN_IF_ERROR(service_pool->Init(num_service_threads));
 :   KUDU_RETURN_IF_ERROR(
> One line ?
Done


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

http://gerrit.cloudera.org:8080/#/c/8439/3/be/src/util/openssl-util.h@31
PS3, Line 31: //
> nit: ///
Done



--
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: comment
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 16 Jan 2018 21:38:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9038/2//COMMIT_MSG@17
PS2, Line 17: Testing: The problem is easily reproduced by lowering the value of
: STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
: with 3 impalads. With the fix, we can see the following entry in 
the
: statestore log:
Is it possible to add a test for this?


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

http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc@353
PS2, Line 353: // Assumes that the caller has taken subscribers_lock_
I would add this comment in the header comment in the .h file.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 16 Jan 2018 21:22:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6399: Increase timeout in test observability to reduce flakiness

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6399: Increase timeout in test_observability to reduce 
flakiness
..


Patch Set 1: Code-Review+2

Pending success of dry-run test.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58f7e7b367e73675be42e85f55fd7698d51f92af
Gerrit-Change-Number: 9034
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 16 Jan 2018 18:06:23 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9006/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9006/3/CMakeLists.txt@310
PS3, Line 310: Tests
> Is this confined to test only ? Won't it also affect Impalad proper if the
It happens only for numeric IP addresses that do not have any associated 
hostnames with reverse DNS. We have a setup like that only for our tests, 
however if a user were to do that (which ideally they wouldn't), this issue 
would show up for them too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
Gerrit-Change-Number: 9006
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 16 Jan 2018 17:43:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for KRPC

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for 
KRPC
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5be574435af51fb7a875b16888cca260b341190e
Gerrit-Change-Number: 8991
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 12 Jan 2018 18:48:09 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

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, 326 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8439/3
--
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: newpatchset
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

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, 327 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/8439/2
--
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: newpatchset
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for KRPC

[Sailesh Mukil (Code Review)]

Sailesh Mukil has removed a vote on this change.

Change subject: IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for 
KRPC
..


Removed Code-Review+2 by Sailesh Mukil 
--
To view, visit http://gerrit.cloudera.org:8080/8991
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I5be574435af51fb7a875b16888cca260b341190e
Gerrit-Change-Number: 8991
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for KRPC

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for 
KRPC
..


Patch Set 3:

> (1 comment)

To avoid extra complication then, wouldn't it be easier to move the 
initialization somewhere else?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5be574435af51fb7a875b16888cca260b341190e
Gerrit-Change-Number: 8991
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 11 Jan 2018 22:21:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9006/2/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9006/2/CMakeLists.txt@310
PS2, Line 310: # Tests that run any security related tests need to link this in 
to override the
> This is pretty opaque. Could you provide a comment here with what's going o
It's pretty well explained in be/src/kudu/security/krb5_realm_override.cc, so I 
added a comment to refer that file.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
Gerrit-Change-Number: 9006
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 11 Jan 2018 19:29:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

Hello Philip Zeyliger,

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

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

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

Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..

IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

On systems that have Kerberos 1.11 or earlier, service principals with
IP addresses are not supported due to a bug:

http://krbdev.mit.edu/rt/Ticket/Display.html?id=7603

Since our BE tests use such principals, they fail on older platforms with the
above mentioned kerberos versions.

Kudu fixed this by adding a workaround which overrides krb5_realm_override.

https://github.com/cloudera/kudu/commit/ba2ae3de4a7c43ff2f5873e822410e066ea99667

However, when we moved Kudu's security library into Impala, we did not
add the appropriate build flags that allow it to be used. This patch fixes
that.

Testing: Verified that the failing test runs successfully on CentOs 6.4
with Kerberos 1.10.3

Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
---
M CMakeLists.txt
M be/src/rpc/CMakeLists.txt
M be/src/rpc/rpc-mgr-test.cc
3 files changed, 9 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
Gerrit-Change-Number: 9006
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for KRPC

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5528: Upgrade GPerfTools to 2.6.3 and tune TCMalloc for 
KRPC
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5be574435af51fb7a875b16888cca260b341190e
Gerrit-Change-Number: 8991
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 11 Jan 2018 19:12:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker,

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

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

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

Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..

IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

In KrpcDataStreamMgr::CreateRecvr() we take the lock_ and
then call recvr->TakeOverEarlySender() for all contexts.
recvr->TakeOverEarlySender() then calls
recvr_->mgr_->EnqueueDeserializeTask((), which can block if the
deserialize pool queue is full. The next thread to become available
in that queue will also have to acquire lock_, thus leading to a
deadlock.

We fix this by moving the EarlySendersList out of the
EarlySendersMap and dropping the lock before taking any actions on
the RPC contexts in the EarlySendersList. All functions called after
dropping 'lock_' do not require the lock to protect them as they are
thread safe.

Additionally modified the BE test data-stream-test to work with KRPC
as well.

Testing: Added a new test to data-stream-test to verify that the
deadlock does not happen. Also, I verified that this test hangs
without the fix.

Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
2 files changed, 262 insertions(+), 46 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
Gerrit-Change-Number: 8950
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

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


Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..

IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

On systems that have Kerberos 1.11 or earlier, service principals with
IP addresses are not supported due to a bug:

http://krbdev.mit.edu/rt/Ticket/Display.html?id=7603

Since our BE tests use such principals, they fail on older platforms with the
above mentioned kerberos versions.

Kudu fixed this by adding a workaround which overrides krb5_realm_override.

https://github.com/cloudera/kudu/commit/ba2ae3de4a7c43ff2f5873e822410e066ea99667

However, when we moved Kudu's security library into Impala, we did not
add the appropriate build flags that allow it to be used. This patch fixes
that.

Change-Id: I60e291e8aa1b59b645b856d33c658471f314c221
---
M CMakeLists.txt
M be/src/rpc/CMakeLists.txt
M be/src/rpc/rpc-mgr-test.cc
3 files changed, 6 insertions(+), 1 deletion(-)



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

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

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker,

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

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

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

Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..

IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

In KrpcDataStreamMgr::CreateRecvr() we take the lock_ and
then call recvr->TakeOverEarlySender() for all contexts.
recvr->TakeOverEarlySender() then calls
recvr_->mgr_->EnqueueDeserializeTask((), which can block if the
deserialize pool queue is full. The next thread to become available
in that queue will also have to acquire lock_, thus leading to a
deadlock.

We fix this by moving the EarlySendersList out of the
EarlySendersMap and dropping the lock before taking any actions on
the RPC contexts in the EarlySendersList. All functions called after
dropping 'lock_' do not require the lock to protect them as they are
thread safe.

Additionally modified the BE test data-stream-test to work with KRPC
as well.

Testing: Added a new test to data-stream-test to verify that the
deadlock does not happen. Also, I verified that this test hangs
without the fix.

Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
---
M be/src/runtime/data-stream-test.cc
M be/src/runtime/krpc-data-stream-mgr.cc
2 files changed, 263 insertions(+), 46 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
Gerrit-Change-Number: 8950
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-6348: Redact only sensitive fields in runtime profiles

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6348: Redact only sensitive fields in runtime profiles
..


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py
File tests/custom_cluster/test_redaction.py:

http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py@336
PS2, Line 336: self.assert_query_profile_contains(self.find_last_query_id(), 
user_profile_pattern)
> You mean L315/L317?  We ran a different query in L335, find_last_query_id()
My bad. Ignore.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae3b6726009bf458a7ec73131e5d659b12ab73cf
Gerrit-Change-Number: 8934
Gerrit-PatchSet: 3
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 05 Jan 2018 23:55:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

[Sailesh Mukil (Code Review)]

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


Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr
..

IMPALA-6346: Potential deadlock in KrpcDataStreamMgr

In KrpcDataStreamMgr::CreateRecvr() we take the lock_ and
then call recvr->TakeOverEarlySender() for all contexts.
recvr->TakeOverEarlySender() then calls
recvr_->mgr_->EnqueueDeserializeTask((), which can block if the
deserialize pool queue is full. The next thread to become available
in that queue will also have to acquire lock_, thus leading to a
deadlock.

We fix this by moving the EarlySendersList out of the
EarlySendersMap and dropping the lock before taking any actions on
the RPC contexts in the EarlySendersList. All functions called after
dropping 'lock_' do not require the lock to protect them as they are
thread safe.

Testing: Manually verified that queries work. It's not easy to write
a deterministic test case to catch this.

Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7
---
M be/src/runtime/krpc-data-stream-mgr.cc
1 file changed, 18 insertions(+), 13 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-6348: Redact only sensitive fields in runtime profiles

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6348: Redact only sensitive fields in runtime profiles
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8934/2//COMMIT_MSG@29
PS2, Line 29: Other fields in the runtime profile are left unredacted.
This implicitly states that we aren't allowed to log anything sensitive in any 
field other than the above 4 mentioned. Is that commented or documented 
somewhere? If not, it would be good to add that.


http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py
File tests/custom_cluster/test_redaction.py:

http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py@336
PS2, Line 336: self.assert_query_profile_contains(self.find_last_query_id(), 
user_profile_pattern)
Should we do this again? It's already done on L313?


http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py@337
PS2, Line 337: assert_query_profile_contains
Why not use assert_web_ui_redaction() here?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae3b6726009bf458a7ec73131e5d659b12ab73cf
Gerrit-Change-Number: 8934
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 04 Jan 2018 21:47:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2228: Make Messenger options configurable

[Sailesh Mukil (Code Review)]

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

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


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8878
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 03 Jan 2018 00:23:36 +
Gerrit-HasComments: No

[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 4: Code-Review+2


--
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: 4
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, 02 Jan 2018 19:49:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5557: Disable rpc default keepalive time ms

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5557: Disable rpc_default_keepalive_time_ms
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0871dd9c9bbe455466b9b2d2a2bbedec79cf0775
Gerrit-Change-Number: 8910
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 02 Jan 2018 18:47:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2228: Make Messenger options configurable

[Sailesh Mukil (Code Review)]

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

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


Patch Set 2: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8878/2/be/src/rpc/authentication.cc@847
PS2, Line 847: keytab_file_
We should mention this in the commit message. It's not just a cherry-pick 
because of this change.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia21814ffb6e283c2791985b089878b579905f0ba
Gerrit-Change-Number: 8878
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 28 Dec 2017 22:19:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5557: Disable rpc default keepalive time ms

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5557: Disable rpc_default_keepalive_time_ms
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0871dd9c9bbe455466b9b2d2a2bbedec79cf0775
Gerrit-Change-Number: 8910
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 28 Dec 2017 18:49:10 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker, Dan Hecht, Impala Public Jenkins,

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

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

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

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.

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

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

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.h
6 files changed, 119 insertions(+), 141 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/9
--
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: 9
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

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

[Sailesh Mukil (Code Review)]

Sailesh Mukil has submitted this change and it was merged. ( 
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
Reviewed-on: http://gerrit.cloudera.org:8080/8471
Reviewed-by: Sailesh Mukil 
Tested-by: Sailesh Mukil 
---
A be/src/rpc/impala-service-pool.cc
A be/src/rpc/impala-service-pool.h
2 files changed, 317 insertions(+), 0 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved; Verified

--
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: merged
Gerrit-Change-Id: I5bb927d4726d4fe418251b9734a4e232810fef69
Gerrit-Change-Number: 8471
Gerrit-PatchSet: 8
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

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

[Sailesh Mukil (Code Review)]

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

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


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

Since the child patch is ready for GVO, manually merging this to avoid the GVO 
job merging only the child without the parent patch (i.e. merging part-2 
without part-1)


--
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: comment
Gerrit-Change-Id: I5bb927d4726d4fe418251b9734a4e232810fef69
Gerrit-Change-Number: 8471
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 07 Dec 2017 14:49:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6262: Always initialize runtime profile for DataSink

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6262: Always initialize runtime profile for DataSink
..


Patch Set 2: Code-Review+1

I'm fine with this patch. Please feel free to upgrade to a +2 if no one else is 
reviewing it.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a683000ef180027b929dbebe78bc2a530a4767e
Gerrit-Change-Number: 8770
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 06 Dec 2017 09:27:03 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 7: Code-Review+2

(1 comment)

Carry +2.

http://gerrit.cloudera.org:8080/#/c/8472/7/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/7/be/src/rpc/impala-service-pool.cc@194
PS7, Line 194: // NOLINT(*)
GVO failed because clang tidy incorrectly wanted the TRACE_TO() macro to fill 
in optional arguments, but it's not necessary here.



--
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: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 20:36:53 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

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

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


Abandoned

Thanks Michael, sounds good. I've uploaded a patch to disable the tests here:
https://gerrit.cloudera.org/#/c/8766/

Abandoning this.
--
To view, visit http://gerrit.cloudera.org:8080/8763
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I5e17d35fa7000bda9aeb8bf2ef950df9d355a9c5
Gerrit-Change-Number: 8763
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

[Sailesh Mukil (Code Review)]

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


Change subject: IMPALA-6268: 
KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing
..

IMPALA-6268: KerberosOnAndOff/RpcMgrKerberizedTest.MultipleServices failing

This patch just disables the failing test to unblock builds.
We will investigate in parallel the root cause for these failures
and post a real fix.

Change-Id: I6c750850ff916617a06e3cfac330072d8e2179e8
---
M be/src/rpc/rpc-mgr-test.cc
1 file changed, 3 insertions(+), 3 deletions(-)



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

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

[Impala-ASF-CR] Revert "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/8763 )

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


Patch Set 2:

> I would recommend against reverting this entire change for the
 > following reasons:
 >
 > - this doesn't break anything on asf-master if KRPC is not enabled.
 > - Our testing with this patch with KRPC enabled has been fine and
 > not hit this problem in the secure cluster.
 > - It's unclear if this is a specific issue related to the test
 > setup (e.g. the way we set up minikdc).
 > - reverting this change will block continuous testing of KRPC.
 >
 > Due to the above, I would recommend only disabling the offending BE
 > test for now.

I've mentioned the reason for the problem in IMPALA-6268. This basically would 
break for KRPC w/ kerberos on any system with kerberos 1.10 or below, due to a 
kerberos bug in those versions. It's not just a test failure, which is why I 
reverted the entire patch instead of disabling the test.

But if you think we can keep the patch in for testing purposes while just 
disabling the test till we find a fix, I'm fine with that too. Let me know what 
you think.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e17d35fa7000bda9aeb8bf2ef950df9d355a9c5
Gerrit-Change-Number: 8763
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 18:58:19 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

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

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

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

This reverts commit d428c16f1e9560af88b326ca19eafecfef2851c2.

The commit caused an issue on platforms with kerberos versions
1.10 and less. Since it's breaking builds, we will revert it and
continue the investigation.

Conflicts:
common/thrift/generate_error_codes.py

Change-Id: I5e17d35fa7000bda9aeb8bf2ef950df9d355a9c5
---
M be/src/common/global-flags.cc
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
D be/src/testutil/mini-kdc-wrapper.cc
D be/src/testutil/mini-kdc-wrapper.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/generate_error_codes.py
14 files changed, 181 insertions(+), 467 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e17d35fa7000bda9aeb8bf2ef950df9d355a9c5
Gerrit-Change-Number: 8763
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] Revert "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/8763


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

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

This reverts commit d428c16f1e9560af88b326ca19eafecfef2851c2.

The commit caused an issue on platforms with kerberos versions
1.10 and less. Since it's breaking builds, we will revert it and
continue the investigation.

Conflicts:
common/thrift/generate_error_codes.py

Change-Id: I5e17d35fa7000bda9aeb8bf2ef950df9d355a9c5
---
M be/src/common/global-flags.cc
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
D be/src/testutil/mini-kdc-wrapper.cc
D be/src/testutil/mini-kdc-wrapper.h
M be/src/util/auth-util.cc
M be/src/util/auth-util.h
M common/thrift/generate_error_codes.py
14 files changed, 180 insertions(+), 466 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-6256: Incorrect principal will be used for internal connections if FLAGS be principal is set

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6256: Incorrect principal will be used for internal 
connections if FLAGS_be_principal is set
..


Patch Set 1:

This depends on an API change from the following cherry-pick from Kudu:
https://gerrit.cloudera.org/#/c/8760/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If5af4398467857da09878075439b6612a04d7a01
Gerrit-Change-Number: 8761
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 06:37:34 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 1:

This is the cherry-pick required for IMPALA-6256. A follow on patch makes use 
of this API:
https://gerrit.cloudera.org/#/c/8761/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695
Gerrit-Change-Number: 8760
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 06:36:26 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,

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

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

to review the following change.


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

[security] Make the kerberos principal configurable for Kudu servers

The Kudu security library currently sources the kerberos principal
directly from FLAGS_principal. Since this is a library, we'd rather
move this to be an option that is passed through InitKerberosForServer().
Users of the security library may pass any principal that they want
to.

Testing: All current tests pass. Since this is a minor refactor, no new
tests are needed.

Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695
Reviewed-on: http://gerrit.cloudera.org:8080/8700
Reviewed-by: Dan Burkert 
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M be/src/kudu/security/init.cc
M be/src/kudu/security/init.h
M be/src/kudu/security/test/mini_kdc-test.cc
3 files changed, 19 insertions(+), 20 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idd16b3360d8d2df5a609eb897bb9810e662fc695
Gerrit-Change-Number: 8760
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[Impala-ASF-CR] IMPALA-6256: Incorrect principal will be used for internal connections if FLAGS be principal is set

[Sailesh Mukil (Code Review)]

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


Change subject: IMPALA-6256: Incorrect principal will be used for internal 
connections if FLAGS_be_principal is set
..

IMPALA-6256: Incorrect principal will be used for internal connections if 
FLAGS_be_principal is set

In Impala, we have FLAGS_principal and FLAGS_be_principal flags.
If only FLAGS_principal is set, we use it as both the internal
and external principals.

If both FLAGS_principal and FLAGS_be_principal are set, we use
FLAGS_be_principal as the internal principal and FLAGS_principal
as the external principal.

However, in Kudu, they only source the internal principal from
FLAGS_principal and aren't aware of a flag called FLAGS_be_principal.
So as of the time IMPALA-5129 went in, if FLAGS_be_principal is
explicitly set to something different from FLAGS_principal, we would
be using the external principal as the internal principal, which is
incorrect.

This is fixed on the Kudu side by the following commit:
https://github.com/apache/kudu/commit/d42c2916467b83347f064ddea59f7a65202f7247

This is now cherry-picked to Impala and we now use the new API to
fix this problem.

Testing: Made the MiniKdc explicitly set FLAGS_principal and
FLAGS_be_principal.

Change-Id: If5af4398467857da09878075439b6612a04d7a01
---
M be/src/rpc/authentication.cc
M be/src/testutil/mini-kdc-wrapper.cc
2 files changed, 9 insertions(+), 3 deletions(-)



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

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

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker, Dan Hecht,

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

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

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

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.

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

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

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.h
6 files changed, 116 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/6
--
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: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: 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 posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

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


Patch Set 6: Code-Review+2

(1 comment)

Rebase, carry +2.

http://gerrit.cloudera.org:8080/#/c/8472/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/5/be/src/rpc/impala-service-pool.h@76
PS5, Line 76: against c
> what resources?  Let's just remove that word since we've been trying to be
Done



--
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: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 05 Dec 2017 05:14:08 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 5:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG@33
PS4, Line 33: This patch however does not display instrumentation on the 
Webpages yet.
: In a future patch, we will add that through the ImpalaServicePool 
and the
: RpcMgr.
> can you file a JIRA for that (and you could reference it here).
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@65
PS4, Line 65:   // TODO: Display these metrics in the debug webpage. IMPALA-6269
:   // Number of RPCs that timed out while waiting in the service 
queue.
:   AtomicInt32 rpcs_timed_out_in_queue_;
:   // Number of RPCs that were rejected due to the queue being 
full.
:   AtomicInt32 rpcs_queue_overflow_;
> these aren't used by anything yet, right? let's add a TODO with a JIRA numb
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@75
PS4, Line 75:
> comment on what that synchronizes
Done. Since the lock theoretically seems unnecessary, I've left a TODO to 
consider removing it later.


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc@40
PS4, Line 40:
: METRIC_DEFINE_histogram(server, impala_unused,
: "RPC Queue Time",
: kudu::MetricUnit::kMic
> include names.h instead
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc@58
PS4, Line 58:
> why do we have that now?
It's not needed. Removed.



--
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: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 21:35:20 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker, Dan Hecht,

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

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

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

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.

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

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr. Tracked by IMPALA-6269

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.h
6 files changed, 116 insertions(+), 138 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/5
--
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: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 1:

Update: This is now blocked on:
https://issues.apache.org/jira/browse/KUDU-2228

The reason is that we need to modify some KRPC gflags at runtime to enable TLS 
w/ KRPC. We found that this may affect the KuduClient as well, which is not 
what we want. To avoid this, we will convert all the flags to messenger options 
and make a modification in this patch to use the Messenger options.


--
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: comment
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 04:59:02 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 4:

The latest patchset does away with the webpage instrumentation for KRPC metrics 
and only exposes the Service Pool threads in the /threadz page.


--
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: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 04 Dec 2017 04:57:13 +
Gerrit-HasComments: No

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 4:

(26 comments)

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@52
PS3, Line 52:   virtual kudu::Status
:   QueueInboundCall(gscoped_ptr call) 
OVERRIDE;
:
:   const std::string service_name() const;
:
:  private:
:   void RunThread();
:
> let's add header function comments to these (and other methods), per usual.
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@69
PS3, Line 69: micInt
> What is this referring to? RPC service methods, or something different?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@72
PS3, Line 72:   // appropriate internal KRPC state. Unused otherwise.
> what is "time" in these cases? wallclock, cpu, ...?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@74
PS3, Line 74:
> what's that? some comments here would help.
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.h@92
PS3, Line 92:
> what does that protect?
Looks like it's left over from a long time ago in the Kudu code, so as far as 
we know it doesn't really protect anything any more.

However, the Kudu folks were skeptical about removing it since it doesn't 
negatively affect anything right now, and we're not sure if there may be some 
issues after removing it.

Let me know what you think.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@182
PS3, Line 182: if (PRED
> Hmm, okay I see we are fishing into the timing_ struct and doing math again
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@186
PS3, Line 186: bably ig
> internal is too vague. Let's "to update the InboundCall timing".  timing()
Done


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@187
PS3, Line 187:
> I guess you're basically doing this manually via time_in_queue computation.
This is the API that KRPC expects. They expect a Histogram to be supplied to 
update the incoming queue time for all RPCs.

Handler latency is a per RPC histogram whereas the incoming queue time 
histogram is over all RPCs.

In any case, I removed all per RPC histograms, so we're just passing the 
unused_histogram_ to satiate the API requirements now.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@212
PS3, Line 212:
> there's no enumeration for the methods? i guess this is okay though.
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@213
PS3, Line 213:
> is that purposely protected by the lock?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@220
PS3, Line 220:
> this is a bit decieving becuase we haven't necessarily actually handled the
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@223
PS3, Line 223:
> why not just do that?
Removed this. N/A


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@240
PS3, Line 240:
> doesn't krpc already have these histograms? is all we want to get out is th
I have a working implementation of getting the histograms from Kudu and using 
it, however, since we decided to defer it, I'm not including it in this patch.

Just to leave a note though, Kudu has the handling time histogram that we need 
to get from them since we cannot accurately calculate that ourselves. They 
payload size histogram and the per RPC queueing time histogram can still be 
calculated by us over here when we decide to do it.

There are a few other interesting metrics like "total number of accepted 
connections", etc. that we could display.


http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/impala-service-pool.cc@255
PS3, Line 255:
> should we also display the total number of service threads even though it's
That's already displayed in the threadz/ page automatically, since all these 
threads are Impala threads.


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

http://gerrit.cloudera.org:8080/#/c/8472/3/be/src/rpc/rpc-mgr.h@a160
PS3, Line 160:
> is that not still true? i.e. since we need to inherit from kudu::RpcService
It actually should be the same. Not sure why I removed that comment. I put it 
back in.

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker, Dan Hecht,

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

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

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

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.

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

This patch however does not display instrumentation on the Webpages yet.
In a future patch, we will add that through the ImpalaServicePool and the
RpcMgr.

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.h
M be/src/service/impalad-main.cc
7 files changed, 116 insertions(+), 136 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/8472/4
--
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: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 

[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 10: Code-Review+2

(1 comment)

Carry +2.

http://gerrit.cloudera.org:8080/#/c/8270/10/be/src/testutil/CMakeLists.txt
File be/src/testutil/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/8270/10/be/src/testutil/CMakeLists.txt@37
PS10, Line 37: target_link_libraries(TestUtil security-test-for-impala)
Build failed because I forgot to link the kudu::MiniKdc with our TestUtil.



--
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: 10
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sat, 02 Dec 2017 01:24:44 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Impala Public Jenkins,

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

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

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

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/common/global-flags.cc
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
M common/thrift/generate_error_codes.py
14 files changed, 466 insertions(+), 180 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/10
--
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: 10
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: 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 (#9).

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/common/global-flags.cc
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
M common/thrift/generate_error_codes.py
14 files changed, 465 insertions(+), 180 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/9
--
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: 9
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 8: Code-Review+2

(2 comments)

Carry +2

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

http://gerrit.cloudera.org:8080/#/c/8270/8/be/src/rpc/rpc-mgr.cc@53
PS8, Line 53: // KuduRPC's flags
: DECLARE_string(rpc_authentication);
> This can be removed now.
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/8/be/src/util/auth-util.cc@93
PS8, Line 93: split(names, names[1], is_any_of("@"));
> Did you double check if it's okay to use the same vector for both input and
The documentation doesn't specify anything. So I just copied it to a new string 
to be safe.



--
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: 8
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 01 Dec 2017 19:58:22 +
Gerrit-HasComments: Yes

[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 7:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@75
PS6, Line 75:   KUDU_RETURN_IF_ERROR(bld.Build(_), "Could not build 
messenger");
> As discussed offline, this flag also affects the Kudu client so it's best t
Done


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@128
PS6, Line 128: nst kudu::rpc::ErrorStatusPB* err = rpc_controller.error
> I see. If this turns out to be necessary, we can consider doing it in the T
I removed this, so it's no longer necessary.



--
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: 7
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 01 Dec 2017 15:59:10 +
Gerrit-HasComments: Yes

[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 (#8).

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/common/global-flags.cc
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
M common/thrift/generate_error_codes.py
14 files changed, 467 insertions(+), 180 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/8
--
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: 8
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] Update incubator-impala -> impala URLs

[Sailesh Mukil (Code Review)]

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

Change subject: Update incubator-impala -> impala URLs
..


Patch Set 1: Code-Review+1

Checked some URLs and confirmed that they work.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie49221300340ef34bdd7c01670c35bdbbce3e84f
Gerrit-Change-Number: 8685
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 29 Nov 2017 17:30:48 +
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 6:

(12 comments)

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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1033
PS6, Line 1033:   
RETURN_IF_ERROR(GetInternalKerberosPrincipal(_internal_principal));
  :   
RETURN_IF_ERROR(GetExternalKerberosPrincipal(_external_principal));
  :   if (!kerberos_internal_principal.empty()) {
  : RETURN_IF_ERROR(InitKerberosEnv());
  :   }
> Does it make sense to write it as:
Done. Using 'bool use_kerberos'


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1055
PS6, Line 1055: !kerberos_internal_principal.empty()
> if (IsKerberosEnabled()) {
Done. I added the DCHECK in the if() condition above, so I didn't redo it here.


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/authentication.cc@1070
PS6, Line 1070: !kerberos_external_principal.empty()
> IsKerberosEnabled(). Should we just declare the following above and use it
Yes, that's much more readable. Done.


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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.h@180
PS6, Line 180:   /// The following strings preserve the Kudu flags original 
values to restore in
 :   /// Shutdown() as they will be modified by us.
 :   string flag_save_rpc_authentication;
> As mentioned in another place, is this actually necessary if we are shuttin
I removed this now.


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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@75
PS6, Line 75: FLAGS_rpc_authentication = "required";
> Meant to say Kudu library code.
Ah I forgot to do this earlier. I now asked the Kudu team about this and the 
feedback was that changing any Kudu flags would affect the client as well. So I 
think we should leave these flags as is, i.e. as the defaults.

I reverted the setting of this flag.


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/rpc/rpc-mgr.cc@128
PS6, Line 128: FLAGS_rpc_authentication = flag_save_rpc_authentication;
> Is there a reason why we have to restore this flag if we are shutting down
I removed this now. But the reason for restoring it was that our tests start 
the RpcMgr multiple times, so we don't want the setting of one RpcMgr config to 
leak into the next.


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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@38
PS6, Line 38: /// If the mode is USE_KUDU_KERBEROS or USE_IMPALA_KERBEROS, the 
MiniKdc which is a wrapper
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@71
PS6, Line 71:   std::string ticket_lifetime_;
> const
Done


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@74
PS6, Line 74:   std::string renew_lifetime_;
> const
Done


http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/testutil/mini-kdc-wrapper.h@77
PS6, Line 77:   int kdc_port_;
> const
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.h@49
PS6, Line 49: /// 'out_principal' will contain the principal string if the 
cluster is configured to use
: /// kerberos, otherwise it will be an empty string.
> As mentioned in auth-util.cc, we may want to call this function only when K
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/6/be/src/util/auth-util.cc@75
PS6, Line 75: FLAGS_principal.empty()
> !IsKerberosEnabled().
Yes, that's right. I've changed it to a DCHECK



--
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: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 29 Nov 2017 17:17:08 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8439/1/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8439/1/be/src/rpc/rpc-mgr.h@183
PS1, Line 183:  /// The following strings preserve the Kudu flags original 
values to restore in
 :   /// Shutdown() as they will be modified by us.
 :   string flag_save_ca_certificate_file;
 :   string flag_save_rpc_private_key_file;
 :   string flag_save_rpc_certificate_file;
 :   string flag_save_rpc_private_key_password_cmd;
 :   string flag_save_rpc_tls_ciphers;
 :   string flag_save_rpc_tls_min_protocol;
> There is no magic here :) My point is you need to trigger the validation so
Ah you're right. I assumed that would have been called through some code path, 
but it looks like that doesn't happen.

I'll try running it as a part of our Init routines and post an updated patch.



--
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: comment
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 28 Nov 2017 22:17:21 +
Gerrit-HasComments: Yes

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

[Sailesh Mukil (Code Review)]

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

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


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8439/1/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8439/1/be/src/rpc/rpc-mgr.h@183
PS1, Line 183:  /// The following strings preserve the Kudu flags original 
values to restore in
 :   /// Shutdown() as they will be modified by us.
 :   string flag_save_ca_certificate_file;
 :   string flag_save_rpc_private_key_file;
 :   string flag_save_rpc_certificate_file;
 :   string flag_save_rpc_private_key_password_cmd;
 :   string flag_save_rpc_tls_ciphers;
 :   string flag_save_rpc_tls_min_protocol;
> But in the tests, wouldn't each test case that creates a messenger need to
We don't set all of them for all the tests. We could set the ones we don't need 
as defaults for the tests that don't need them, but the defaults of some of 
these flags are pretty large. Eg, see the FLAGS_rpc_tls_ciphers:
https://github.com/apache/incubator-impala/blob/32baa695f499a936b72c5a51ae3649c408aa5a85/be/src/kudu/security/tls_context.cc#L53-L70

So it would be odd setting it to this hardcoded value before every test.

Some flags are only used by the builder but others are used at runtime too.

The validators are automatically run while creating Messenger objects.



--
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: comment
Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a
Gerrit-Change-Number: 8439
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 28 Nov 2017 18:31:22 +
Gerrit-HasComments: Yes

[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 6:

(13 comments)

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

http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/auth-provider.h@27
PS5, Line 27: #include "util/thread.h"
> What is this for ?
This is for the 'kinit_thread_' below. We hadn't IWYU'd before.


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

http://gerrit.cloudera.org:8080/#/c/8270/4/be/src/rpc/auth-provider.h@78
PS4, Line 78:   /// Wrap the client transport with a new TSaslClientTransport.  
This is only for
:   /// internal connections.  Since, as a daemon, we only do 
Kerberos and not LDAP,
:   /// we can go straight to Kerberos.
:   /// This is only applicable to Thrift connections and not KRPC 
connections.
:   virtual Status WrapClientTransport(const std::string& hostname,
:   boost::shared_ptr 
raw_transport,
:   const std::string& service_name,
> Is this only used for thrift connections ? May be it helps to state it now
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@643
PS5, Line 643: // Kudu Client and Kudu RPC shouldn't attempt to initialize 
SASL which would conflict
 : // with Impala's SASL initialization. This must be call
> Please add a small comment that this overlaps with the initialization below
Done


http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@658
PS5, Line 658: sasl::TSaslServer::SaslInit(GENERAL_CALLBACKS, appname);
 : sasl::TSaslClient::SaslInit(GENERAL_CALLBACKS);
 :   } catch (sasl::SaslServerImplException& e) {
> Once we completely move away from Thrift, do we rely on Kudu to initialize
We will still have Thrift for external connections, so my take is that we can 
leave all the SASL initialization in Impala's control so we can add the kinds 
of callbacks, etc. that we like.


http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@1034
PS5, Line 1034:   
RETURN_IF_ERROR(GetExternalKerberosPrincipal(_external_principal));
  :   if (!kerberos_internal_principal.empty()) {
> The internal and external principals look like good candidates to be stored
We can do that, however, I think it adds some unnecessary complexity for the 
authentication classes to depend on the ExecEnv.

For eg, some of the backend tests (including the rpc-mgr-test and 
thrift-server-test) do not have an ExecEnv but use the authentication classes. 
In that case, we'd need to add ExecEnv objects to the tests just for this.

Let me know what you think.


http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/authentication.cc@1036
PS5, Line 1036: RETURN_IF_ERROR(InitKerberosEnv());
> nit: blank line not needed.
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/rpc-mgr.cc@63
PS5, Line 63: FLAGS_num_reactor_thread
> IsKerberosEnabled()
Done


http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/rpc/rpc-mgr.cc@70
PS5, Line 70: string service_name, unused_hostname, unused_realm;
> It may be better to set FLAGS_rpc_authentication (defined by Kudu) to "requ
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.h@53
PS5, Line 53: //
> nit: ///
Done


http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.h@86
PS5, Line 86:   /// Stops the KDC by terminating the krb5kdc subprocess.
> Please add comments about what clean up it may do.
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.cc@67
PS5, Line 67: !
> != seems to make less assumption about the ordering of the ENUM values.
Done


http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/testutil/mini-kdc-wrapper.cc@88
PS5, Line 88: !
> !=
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/5/be/src/util/auth-util.cc@86
PS5, Line 86:   split(names, principal, is_any_of("/"));
:   if (names.size() != 2) return 
Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal);
:
> Should this live in generate_error_codes.py ?
Done. Added to generate_error_codes.



--
To view, visit 

[Impala-ASF-CR] IMPALA-6201: Fix test basic filters on ASAN

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6201: Fix test_basic_filters on ASAN
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8c20cbb75a9b6da73137f220657aa75dea9dfdce
Gerrit-Change-Number: 8646
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 27 Nov 2017 18:34:57 +
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert

[Sailesh Mukil (Code Review)]

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

Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8622/1//COMMIT_MSG@7
PS1, Line 7: KUDU-2220: GetEndOfChainX509 does not return end-user cert
> What about mentioning IMPALA-6172 in the commit message header?
Yes, I think we can just make IMPALA-6172 as a dup of KUDU-2220 instead of 
modifying the commit message.

We haven't modified the commit messages unless we for some reason or the other 
had to modify the cherry-pick'd patch itself.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8622
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:33:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert

[Sailesh Mukil (Code Review)]

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

Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
..


Patch Set 1:

This patch fixes IMPALA-6172.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8622
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:19:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert

[Sailesh Mukil (Code Review)]

Sailesh Mukil has removed Todd Lipcon from this change.  ( 
http://gerrit.cloudera.org:8080/8622 )

Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
..


Removed reviewer Todd Lipcon.
--
To view, visit http://gerrit.cloudera.org:8080/8622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8622
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert

[Sailesh Mukil (Code Review)]

Sailesh Mukil has removed Alexey Serbin from this change.  ( 
http://gerrit.cloudera.org:8080/8622 )

Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
..


Removed reviewer Alexey Serbin.
--
To view, visit http://gerrit.cloudera.org:8080/8622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8622
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert

[Sailesh Mukil (Code Review)]

Sailesh Mukil has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/8622 )

Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
..


Removed reviewer Kudu Jenkins.
--
To view, visit http://gerrit.cloudera.org:8080/8622
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8622
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] KUDU-2220: GetEndOfChainX509 does not return end-user cert

[Sailesh Mukil (Code Review)]

Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

to review the following change.


Change subject: KUDU-2220: GetEndOfChainX509 does not return end-user cert
..

KUDU-2220: GetEndOfChainX509 does not return end-user cert

KUDU-2091 introduced a function GetEndOfChainX509() which was supposed
to return the "end-user" certificate. However, the end-user certificate
is not at the end of the chain, but rather at the beginning of the chain
as specificed by the RFC:
https://tools.ietf.org/html/rfc5246#section-7.4.2

  | This is a sequence (chain) of certificates. The sender's certificate MUST
  | come first in the list. Each following certificate MUST directly certify
  | the one preceding it.

This patch fixes this by changing the GetEndOfChainX509() to
GetTopOfChainX509(). An existing test is modified to test this patch. It does
not pass without this change.

Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Reviewed-on: http://gerrit.cloudera.org:8080/8595
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
Reviewed-by: Todd Lipcon 
---
M be/src/kudu/rpc/rpc-test.cc
M be/src/kudu/security/ca/cert_management.cc
M be/src/kudu/security/cert.cc
M be/src/kudu/security/cert.h
M be/src/kudu/security/test/test_certs.cc
M be/src/kudu/security/tls_context.cc
M be/src/kudu/security/tls_handshake.cc
7 files changed, 65 insertions(+), 22 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0e3f913259ec4c855ff211726fa6ecea94d328e7
Gerrit-Change-Number: 8622
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[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 7:

> Change has been successfully cherry-picked as 
> fb4c3b01240d8f65fc2c45bf27b668ae9b1fa5d2
 > by Impala Public Jenkins

Thanks for the patch Xianda. The patch has been merged.


--
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: 7
Gerrit-Owner: Xianda Ke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mike Yoder 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xianda Ke 
Gerrit-Comment-Date: Sun, 19 Nov 2017 22:30:34 +
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 4:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8270/4/be/src/rpc/authentication.cc@593
PS4, Line 593: !FLAGS_principal.empty()
> What do you think about wrapping this inside an inline function ?
I wrapped it in a function, but I can't keep it inline, because inline 
functions need to be defined in the .h file (unless there's some other way to 
do it) and we can't access gflags members in the .h file.


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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@90
PS3, Line 90: }
> It seems better to at least make sure / appears before @.
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: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Sun, 19 Nov 2017 04:14:14 +
Gerrit-HasComments: Yes

[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 (#5).

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/common/global-flags.cc
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
13 files changed, 448 insertions(+), 172 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/5
--
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: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-6109: xfail TestHdfsUnknownErrors::test hdfs safe mode error 255

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6109: xfail 
TestHdfsUnknownErrors::test_hdfs_safe_mode_error_255
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55979bed07147409949b798d4beb7a3b3b7ec5c3
Gerrit-Change-Number: 8590
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 17 Nov 2017 23:30:12 +
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 3:

(15 comments)

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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@63
PS3, Line 63: if (!FLAGS_principal.empty() || !FLAGS_be_principal.empty()) {
> Actually, https://github.com/apache/incubator-impala/blob/master/be/src/rpc
You're right, FLAGS_principal is the "switch" to turn on kerberos. So there's 
no reason to check be_principal here. Done.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@64
PS3, Line 64: std::string
> string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/rpc-mgr.cc@67
PS3, Line 67: std::string
> string
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/rpc/thrift-server-test.cc@99
PS3, Line 99: kdc_wrapper_.reset(new MiniKdcWrapper("impala/localhost", 
"KRBTEST.COM", "24h", "7d", kdc_port));
> nit: long line
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@48
PS3, Line 48: backend connections
> Actually, does this may also apply to impala <-> statestore connections ?
This applies to all daemon-daemon connections, so that includes impalad, 
statestored and catalogd.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@49
PS3, Line 49: string
> principal string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@54
PS3, Line 54:
> nit: blank space
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@62
PS3, Line 62: DissectKerberosPrincipal
> Will ParseKerberosPrincipal() a more appropriate name ?
Yup, changed it.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.h@63
PS3, Line 63:
> nit: blank space
Done


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

http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@55
PS3, Line 55: std::string
> nit: string.
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@66
PS3, Line 66: std::string
> nit: string
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@67
PS3, Line 67:   *out_principal = std::string();
:   if (FLAGS_principal.empty()) return Status::OK();
:   *out_principal = FLAGS_principal;
> Is there any reason we cannot just write:
No, I changed it.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@76
PS3, Line 76:  if (FLAGS_principal.empty()) return Status::OK();
> Please see comments in rpc-mgr.cc. Not sure if it's possible to have the ca
As mentioned there, FLAGS_principal is the switch to turn on kerberos, so 
setting FLAGS_be_principal without setting FLAGS_principal is a startup error.


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@77
PS3, Line 77:   if (FLAGS_be_principal.empty()) {
: *out_principal = FLAGS_principal;
: RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal));
:   } else {
: *out_principal = FLAGS_be_principal;
: RETURN_IF_ERROR(ReplacePrincipalHostFormat(out_principal));
:   }
> *out_principal = !FLAGS_be_principal.empty() ? FLAGS_be_principal : FLAGS_p
Done


http://gerrit.cloudera.org:8080/#/c/8270/3/be/src/util/auth-util.cc@90
PS3, Line 90: split(names, principal, is_any_of("/@"));
> Will this generate wrong result if the input principal is of the form '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, 17 Nov 2017 00:28:41 +
Gerrit-HasComments: Yes

[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 (#4).

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/common/global-flags.cc
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
13 files changed, 439 insertions(+), 176 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8270/4
--
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: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] Impala is graduating; remove outdated references to incubation

[Sailesh Mukil (Code Review)]

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

Change subject: Impala is graduating; remove outdated references to incubation
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e6080a2b196926e46b1e641f6530ba1fa9bd444
Gerrit-Change-Number: 8577
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 16 Nov 2017 19:02:15 +
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Impala graduated; remove outdated references to incubation

[Sailesh Mukil (Code Review)]

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

Change subject: Impala graduated; remove outdated references to incubation
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I6fc2852659a0aa6fcb6e6f5f9cf0cfb725c26a84
Gerrit-Change-Number: 8575
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 16 Nov 2017 19:01:25 +
Gerrit-HasComments: No