[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> Sounds like we're missing a test?
The behavior is covered in this test we are commenting on. I didn't think it 
through and just removed TLSv1_*_plus from this test and that is how Michael 
found it wrong.

By the way, if we patch thrift so that TLSv1_1 works as TLSv1_1_plus in thrift 
0.9.0, this test might become meaningless - any combination will work because 
they will all negotiate into using TLS 1.2. So we might have to remove this 
test.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 19 Apr 2018 22:44:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> Talked to Tianyi in person about this.
Sounds like we're missing a test?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 19 Apr 2018 22:33:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Michael Ho (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> My understanding of the test is that the thrift server started with certain
Talked to Tianyi in person about this.

This patch as-is will  break compatibility. In particular, the code in Thrift 
(https://github.com/apache/thrift/blob/0.9.3/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L139-L153)
 will only support the specified TLS version.

In other words, if we specify FLAGS_minimum_ssl_version as "tlsv1", a client 
used to be able to speak to the server using TLSv1, TLSv1.1 and TLSv1.2 in the 
old code. With this patch, a client can only talk to the thrift server using 
TLSv1 as the thrift code now uses TLSv1_method() which only understands TLSv1 
(https://www.openssl.org/docs/man1.0.2/ssl/SSL_CTX_new.html).

As Tianyi suggested, patching Thrift is one way of preserving this 
compatibility.

Sailesh, what do you think ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 19 Apr 2018 22:26:39 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Michael Ho (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> I realized I didn't think it through.
My understanding of the test is that the thrift server started with certain 
version of OpenSSL should be backward compatible with client using <= server 
version.

So if the server is using TLSv1_1, I would expect the whitelist to be SSLTLS, 
TLSv1_0 and TLSv1_1.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 19 Apr 2018 22:03:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> I think if we take option 1, the implementation should be:
Please ignore the last comment there is no process-wide setting.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 18 Apr 2018 19:12:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> I realized I didn't think it through.
I think if we take option 1, the implementation should be:
When impala initializes, read ssl_minimum_version and disable versions lower 
than the minimum process-wide. Then always use SSLTLS to initialize ssl 
context. This way we don't need to patch thrift. And this test needs to be 
refactored a little.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 18 Apr 2018 02:25:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(4 comments)

Sailesh, can you have a look at the TLS behavior in thrift-server-test.cc?

http://gerrit.cloudera.org:8080/#/c/9300/11/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9300/11/CMakeLists.txt@159
PS11, Line 159: 1.0.1
> Is the plan to also cherry-pick the change to 2.x branch ?
I think the current consensus is that it's not for 2.x.


http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/common/init.cc@240
PS11, Line 240: signal(SIGPIPE, SIG_IGN);
> Why is this suddenly needed as part of the upgrade to thrift 0.9.3 ?
I hit this when running tests and didn't look deeper into it. I believe it's 
always needed. (As an example, kudu always ignores this signal.)


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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
> Should this also include TLSv1_0 ?
I realized I didn't think it through.
The current behavior of this test in this patch doesn't make sense but I don't 
really know what behavior we want here. TLSv*_plus are removed in thrift 0.9.3. 
We have several options here:
1. Make TLSv* behave like TLSv*_plus in 0.9.0. In that case, any combination 
would work in this test, because the client and server can always agree on 
TLSv1_2.
2. Patch thrift and add Tlsv*_plus back.

The current behavior in this patch is that TLSv1_1 can only connect to TLSv1_1, 
while SSLTLS can connect to anything. This is different from impala's existing 
behavior and should be changed.
I think option 1 is OK. I think we want the configuration to be flexible but 
respects the tls minimum version constraint, and option 1 should deliver that.


http://gerrit.cloudera.org:8080/#/c/9300/11/common/thrift/CMakeLists.txt
File common/thrift/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9300/11/common/thrift/CMakeLists.txt@68
PS11, Line 68: "ImpalaService.thrift"
> So, is the generated java file of ImpalaService.thrift not needed in the FE
That file defines the impalad RPC interface and is not used in FE.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 18 Apr 2018 01:59:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Michael Ho (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 11:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/9300/11/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9300/11/CMakeLists.txt@159
PS11, Line 159: 1.0.1
Is the plan to also cherry-pick the change to 2.x branch ?

I believe there may be more complication downstream if that's the plan.


http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/common/init.cc@240
PS11, Line 240: signal(SIGPIPE, SIG_IGN);
Why is this suddenly needed as part of the upgrade to thrift 0.9.3 ?


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

http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@362
PS11, Line 362:   {TLSv1_1, {SSLTLS, TLSv1_1}},
Should this also include TLSv1_0 ?


http://gerrit.cloudera.org:8080/#/c/9300/11/be/src/rpc/thrift-server-test.cc@363
PS11, Line 363:   {TLSv1_2, {SSLTLS, TLSv1_2}}};
Should this also include TLSv1_0 and TLSv1_1 ?


http://gerrit.cloudera.org:8080/#/c/9300/11/common/thrift/CMakeLists.txt
File common/thrift/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9300/11/common/thrift/CMakeLists.txt@68
PS11, Line 68: "ImpalaService.thrift"
So, is the generated java file of ImpalaService.thrift not needed in the FE all 
along ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Mon, 16 Apr 2018 19:31:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#11). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 77 insertions(+), 174 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 10:

I think it would be good to have Michael Ho do a review in addition to Sailesh 
(who I see has completed his).


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 03 Apr 2018 23:39:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#10). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 77 insertions(+), 174 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/9300/10
--
To view, visit http://gerrit.cloudera.org:8080/9300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 9: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 14 Mar 2018 21:14:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#9). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 77 insertions(+), 174 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 8:

(2 comments)

Apologies for the slow review.

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h@323
PS7, Line 323: TLSv1_0
> I don't understand why it's equivalent. In 0.9.0 TLSv1_method() is called h
We patched that a while back:
https://gerrit.cloudera.org/#/c/7558/

So it's actually calling SSLv23_method()


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

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@99
PS7, Line 99: return true;
> Do we still need SSLTLS here?
Hmm, you're right. Let's leave that condition there since that's the default.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 13 Mar 2018 18:21:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h@323
PS7, Line 323: TLSv1_0
> Done
I don't understand why it's equivalent. In 0.9.0 TLSv1_method() is called here 
https://github.com/apache/thrift/blob/0.9.x/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L60,
 but SSLTLS in 0.9.3 correspond to TLSv23_method().


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

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@99
PS7, Line 99: return protocol != SSLv3 && protocol != TLSv1_2;
> I think we still need SSLTLS at https://gerrit.cloudera.org/c/9300/7/be/src
Do we still need SSLTLS here?
 https://gerrit.cloudera.org/c/9300/7/be/src/rpc/thrift-server.h#323



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 09 Mar 2018 19:43:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 8:

(2 comments)

Once you address my comments below, please rebase with the master branch as 
that has KRPC enabled by default and make sure that both secure and non-secure 
environments run properly.

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

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@77
PS7, Line 77: r (const auto& prot
> Removed. Thrift added this enum and I was not sure whether it should be her
I just realized. We need to maintain parity with KRPC as well:
https://github.com/apache/kudu/blob/2a5a12fbd69882b387f83e1da71d8a0fd19b6a63/src/kudu/server/server_base.cc#L168-L173

This means that we cannot use the SSLTLS option too. Let's just stick with 
TLSv1_0, TLSv1_1, and TLSv1_2.

In that case, we can leave the 'ssl_minimum_version' flag as it is.


http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@99
PS7, Line 99: return true;
> I mean  protocol == SSLTLS || protocol == TLSv1_0 || protocol == TLSv1_1
Yup, this is right, but will now need to change anyway because we'll need to 
get rid of SSLTLS.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:37:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 78 insertions(+), 174 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h@323
PS7, Line 323: TLSv1_0
> To not change behavior, let's use the equivalent of TLSv1_0_plus from our o
Done


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

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@77
PS7, Line 77: {"latest", LATEST}}
> Where is this specified? Or is this a new option that we're trying to expos
Removed. Thrift added this enum and I was not sure whether it should be here. 
Should we change the definition of ssl_minimum_version, because, I assume, 
there isn't xxx_plus anymore?


http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@97
PS7, Line 97: return protocol != SSLv3 && protocol == TLSv1_0;
> return protocol == SSLTLS || protocol == TLSv1_0;
Done


http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@99
PS7, Line 99: return protocol != SSLv3 && protocol != TLSv1_2;
> return protocol == SSLTLS || protocol == TLSv1_1 && protocol != TLSv1_2;
Shouldn't this be protocol == SSLTLS || protocol == TLSv1_0 || protocol != 
TLSv1_1?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 06 Mar 2018 02:42:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h
File be/src/rpc/thrift-server.h:

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.h@323
PS7, Line 323: TLSv1_0
To not change behavior, let's use the equivalent of TLSv1_0_plus from our old 
toolchain. In Thrift-0.9.3, that's SSLTLS.

https://github.com/apache/thrift/blob/0.9.3/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L140


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

http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@77
PS7, Line 77: {"latest", LATEST}}
Where is this specified? Or is this a new option that we're trying to expose?


http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@97
PS7, Line 97: return protocol != SSLv3 && protocol == TLSv1_0;
return protocol == SSLTLS || protocol == TLSv1_0;


http://gerrit.cloudera.org:8080/#/c/9300/7/be/src/rpc/thrift-server.cc@99
PS7, Line 99: return protocol != SSLv3 && protocol != TLSv1_2;
return protocol == SSLTLS || protocol == TLSv1_1 && protocol != TLSv1_2;



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Mon, 05 Mar 2018 23:22:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 7:

> Patch Set 6:
>
> (1 comment)

Thanks. It seems to be a circular pipe created by CLONE_FILES that caused the 
hang, not waiting for a SIGPIPE. It should be fine even if the pipe is not 
manually closed in that breakpad patch. Ignoring SIGPIPE should be OK in impala.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Mon, 05 Mar 2018 21:50:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#7). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 82 insertions(+), 174 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[anujphadke (Code Review)]

anujphadke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9300/6/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/9300/6/be/src/common/init.cc@240
PS6, Line 240:   signal(SIGPIPE, SIG_IGN);
Could ignoring SIGPIPE have any side effects?

E.g -
IMPALA-5187 depends on Breakpad #728:
When writing a minidump on Linux, Breakpad called clone() in
linux/handler/exception_handler.cc with the CLONE_FILES flag. If the
parent process died while the child waited for the continuation signal,
the write side of the pipe 'fdes' stayed open in the child. The child
would not receive a SIGPIPE and would wait forever.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 02 Mar 2018 22:29:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#6). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 82 insertions(+), 174 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9300/4/be/src/common/init.cc
File be/src/common/init.cc:

http://gerrit.cloudera.org:8080/#/c/9300/4/be/src/common/init.cc@239
PS4, Line 239: .
formatting


http://gerrit.cloudera.org:8080/#/c/9300/4/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/9300/4/bin/impala-config.sh@75
PS4, Line 75: 40-922a6d71e8
I don't think this is necessary - the toolchain has already been bumped to a 
version that's more recent than this (in fact, this probably excludes the 
latest bump in Kudu version which is needed for DECIMAL support, and so this 
probably doesn't compile if rebased on master).

My understanding is that currently the toolchain builds both 0.9.0-p11 (the 
current thrift version) and 0.9.3-p3 (the new thrift version), which is how 
we're already building against an updated toolchain without having bumped 
IMPALA_THRIFT_VERSION below.

Correct me if any of that is wrong.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Tue, 20 Feb 2018 21:24:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
14 files changed, 83 insertions(+), 175 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/coordinator.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
16 files changed, 85 insertions(+), 177 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9300 )

Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/coordinator.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
16 files changed, 85 insertions(+), 177 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9300


Change subject: IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3
..

IMPALA-5690: Part 2: Upgrade thrift to 0.9.3-p3

Dependency changes:
- BE and python use thrift 0.9.3-p3 from native-toolchain.
- FE uses thrift 0.9.3 from apache maven repo.
- Fb303 and http components dependencies are no longer needed in FE and
  are removed.
- The minimum openssl version requirement is increased to 1.0.1.

Configuration change:
- Thrift codegen option movable_type is enabled. New code no longer
  needs to use std::swap to avoid copying.

Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
---
M CMakeLists.txt
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/coordinator.cc
M bin/impala-config.sh
M buildall.sh
M common/thrift/CMakeLists.txt
M fe/pom.xml
M infra/python/deps/compiled-requirements.txt
16 files changed, 85 insertions(+), 177 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I639227721502eaa10398d9490ff6ac63aa71b3a6
Gerrit-Change-Number: 9300
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang