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

2018-02-09 Thread Thomas Tauber-Marshall (Code Review) 
Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9168 )

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


Patch Set 5:

Since this is a very large, complicated patch, is it possible to break it up to 
make it easier to review?

In particular, I think that all of the changes around '<<' could be done in an 
initial patch, and then have a followup patch that contains the actual version 
bump and other things.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 09 Feb 2018 20:01:14 +
Gerrit-HasComments: No

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

2018-02-08 Thread Tianyi Wang (Code Review) 
Tianyi Wang has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/9168 )

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

IMPALA-5690: 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.

Notable code change:
- Thrift 0.9.3 implements "ostream& operator<<(ostream&, T)" for thrift
  data types. It conflicts with impala-defined functions. This patch
  removes these impala-defined functions and changes the call sites
  accordingly.

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

Change-Id: I9c303997411237e988ef960157f781776f6fcb60
---
M CMakeLists.txt
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-metadata-utils.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/thrift-client.cc
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/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/service/child-query.cc
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-server.cc
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-result-set.cc
M be/src/statestore/statestore.cc
M be/src/util/collection-metrics.h
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/histogram-metric.h
M be/src/util/metrics.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/webserver.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
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/shell/test_shell_commandline.py
63 files changed, 336 insertions(+), 443 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang

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

2018-02-08 Thread Philip Zeyliger (Code Review) 
Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9168 )

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


Patch Set 4:

Thomas: could you give this a review?

Sailesh: please review the OpenSSL-related stuff!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Thu, 08 Feb 2018 17:31:16 +
Gerrit-HasComments: No

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

2018-02-08 Thread Philip Zeyliger (Code Review) 
Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9168 )

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


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9168/4/be/src/common/init.cc@236
PS4, Line 236:   // There is no way to pass MSG_NOSIGNAL into OpenSSL so the 
signal must be disabled.
Tell us more about this? Who sends SIGPIPE?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Thu, 08 Feb 2018 17:30:40 +
Gerrit-HasComments: Yes

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

2018-02-07 Thread Tianyi Wang (Code Review) 
Tianyi Wang has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/9168 )

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

IMPALA-5690: 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.

Notable code change:
- Thrift 0.9.3 implements "ostream& operator<<(ostream&, T)" for thrift
  data types. It conflicts with impala-defined functions. This patch
  removes these impala-defined functions and changes the call sites
  accordingly.

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

Change-Id: I9c303997411237e988ef960157f781776f6fcb60
---
M CMakeLists.txt
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/common/init.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-metadata-utils.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/thrift-client.cc
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/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/service/child-query.cc
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-server.cc
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-result-set.cc
M be/src/statestore/statestore.cc
M be/src/util/collection-metrics.h
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/histogram-metric.h
M be/src/util/metrics.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/webserver.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
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/shell/test_shell_commandline.py
63 files changed, 333 insertions(+), 443 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang

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

2018-02-05 Thread Tianyi Wang (Code Review) 
Tianyi Wang has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/9168 )

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

IMPALA-5690: 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.

Notable code change:
- Thrift 0.9.3 implements "ostream& operator<<(ostream&, T)" for thrift
  data types. It conflicts with impala-defined functions. This patch
  removes these impala-defined functions and changes the call sites
  accordingly.

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

Change-Id: I9c303997411237e988ef960157f781776f6fcb60
---
M CMakeLists.txt
M be/src/benchmarks/scheduler-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/exec/exchange-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-metadata-utils.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/thrift-client.cc
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/rpc/thrift-util.h
M be/src/runtime/client-cache.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-mgr.cc
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-sender.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/scheduler.cc
M be/src/service/child-query.cc
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-internal-service.cc
M be/src/service/impala-server.cc
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-result-set.cc
M be/src/statestore/statestore.cc
M be/src/util/collection-metrics.h
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/histogram-metric.h
M be/src/util/metrics.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
M be/src/util/webserver.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
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/shell/test_shell_commandline.py
62 files changed, 330 insertions(+), 443 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang

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

2018-01-31 Thread Tianyi Wang (Code Review) 
Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9168 )

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


Patch Set 1:

> (5 comments)
 >
 > I'm not really qualified to review this change, but what I read
 > looked good to me. Sailesh, please look at the SSL stuff in
 > particular?
 >
 > Do we worry that somebody will accidentally use < a Thrift object? I saw you converted a bunch of uses here. What
 > would happen if someone did LOG(INFO) << thriftObject.foo()?

I think accidentally calling << on a thrift object in logging is fine. The 
output is readable, and you will be able to notice it when you are adding 
logging code. The downside of actually using it is that thrift may change it 
from version to version.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Thu, 01 Feb 2018 02:39:59 +
Gerrit-HasComments: No

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

2018-01-31 Thread Philip Zeyliger (Code Review) 
Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9168 )

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


Patch Set 1:

(5 comments)

I'm not really qualified to review this change, but what I read looked good to 
me. Sailesh, please look at the SSL stuff in particular?

Do we worry that somebody will accidentally use * = nullptr>
: const char* PrintQueryOptionValue(const T& option) {
:   return PrintThriftEnum(option);
: }
:
: template::value>* = nullptr>
: std::string PrintQueryOptionValue(const T& option)  {
:   return std::to_string(option);
: }
Could you add a comment here about what this is doing?


http://gerrit.cloudera.org:8080/#/c/9168/1/common/thrift/CMakeLists.txt
File common/thrift/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/9168/1/common/thrift/CMakeLists.txt@64
PS1, Line 64:   set(CPP_ARGS -r ${THRIFT_INCLUDE_DIR_OPTION} --gen 
cpp:moveable_types -o
Would this be simpler as

set(CPP_ARGS -r ${CPP_ARGS})

Mind you, I don't know CMake enough to be clear on that.


http://gerrit.cloudera.org:8080/#/c/9168/1/common/thrift/CMakeLists.txt@73
PS1, Line 73:   # compile with hive if the thrift version in hive is 0.9.0
I don't understand this one. Isn't Hive using 0.9.3?


http://gerrit.cloudera.org:8080/#/c/9168/1/testdata/workloads/functional-query/queries/QueryTest/set.test
File testdata/workloads/functional-query/queries/QueryTest/set.test:

http://gerrit.cloudera.org:8080/#/c/9168/1/testdata/workloads/functional-query/queries/QueryTest/set.test@20
PS1, Line 20: 'EXPLAIN_LEVEL','STANDARD','REGULAR'
This surprised me.  I don't know if impala-shell is somehow depending on this 
being the numeric enum. (I think this is an improvement, but I wouldn't have 
thought it fell out of this change.)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c303997411237e988ef960157f781776f6fcb60
Gerrit-Change-Number: 9168
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 01 Feb 2018 01:06:23 +
Gerrit-HasComments: Yes