You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2022/06/23 15:33:30 UTC

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18661


Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................

IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

This patch upgrade IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 to mitigate
CVE-2020-13949 and hang issue with newer JDBC client.
IMPALA_TOOLCHAIN_BUILD_ID is upgraded to 179-9977806f06, which contain
the required thrift-0.16.0 compiler.

The refactoring itself includes removing non-generated (empty)
*_constants.cpp and adjustment for THRIFT-4730. It also adjust error
handling in client-request-state.cc due to changing thrift error message
after upgrade.

Testing:
- Build and pass core tests.

Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/service/client-request-state.cc
M be/src/util/process-state-info.cc
M bin/impala-config.sh
10 files changed, 19 insertions(+), 50 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................

IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

This patch upgrades IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 to mitigate
CVE-2020-13949 and hang issue with newer JDBC client.
IMPALA_TOOLCHAIN_BUILD_ID is upgraded to 179-9977806f06, which contains
the required thrift-0.16.0 compiler.

The refactoring itself includes removing non-generated (empty)
*_constants.cpp and adjustment for THRIFT-4730. It also adjusts error
handling in client-request-state.cc due to changing thrift error message
after the upgrade.

Testing:
- Build and pass core tests.

Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/service/client-request-state.cc
M be/src/transport/THttpTransport.cpp
M be/src/transport/THttpTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/transport/TSaslTransport.h
M be/src/util/process-state-info.cc
M bin/impala-config.sh
14 files changed, 23 insertions(+), 54 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................

IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

This patch upgrades IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 to mitigate
CVE-2020-13949 and hang issue with newer JDBC client.
IMPALA_TOOLCHAIN_BUILD_ID is upgraded to 179-9977806f06, which contains
the required thrift-0.16.0 compiler.

The refactoring itself includes:
- Removing non-generated (empty) *_constants.cpp and adjustment for
  THRIFT-4730.
- Adjusts error handling in client-request-state.cc due to changing
  thrift error message after the upgrade.
- Adding custom build target thrift-cpp-manual-edit to fix
  clang-diagnostic-inconsistent-missing-override warning in generated
  ImpalaHiveServer2Service.h

Testing:
- Build and pass core tests.

Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/service/client-request-state.cc
M be/src/transport/THttpTransport.cpp
M be/src/transport/THttpTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/transport/TSaslTransport.h
M be/src/util/process-state-info.cc
M bin/impala-config.sh
M common/thrift/CMakeLists.txt
15 files changed, 32 insertions(+), 55 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 5: Code-Review+1

This is making sense to me. I started a precommit job to see how it goes.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 24 Jun 2022 18:49:55 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 2:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/10851/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 17:28:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................

IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

This patch upgrades IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 to mitigate
CVE-2020-13949 and hang issue with newer JDBC client.
IMPALA_TOOLCHAIN_BUILD_ID is upgraded to 179-9977806f06, which contains
the required thrift-0.16.0 compiler.

The refactoring itself includes:
- Removing non-generated (empty) *_constants.cpp and adjustment for
  THRIFT-4730.
- Adjusts error handling in client-request-state.cc due to changing
  thrift error message after the upgrade.
- Adding custom build target thrift-cpp-manual-edit to fix
  clang-diagnostic-inconsistent-missing-override warning in generated
  ImpalaHiveServer2Service.h and ImpalaService.h

Testing:
- Build and pass core tests.

Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/service/client-request-state.cc
M be/src/transport/THttpTransport.cpp
M be/src/transport/THttpTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/transport/TSaslTransport.h
M be/src/util/process-state-info.cc
M bin/impala-config.sh
M common/thrift/CMakeLists.txt
15 files changed, 38 insertions(+), 60 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 1: Code-Review+1

(2 comments)

lgtm!

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

http://gerrit.cloudera.org:8080/#/c/18661/1//COMMIT_MSG@15
PS1, Line 15: adjust
nit: adjusts


http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/runtime/io/disk-io-mgr.cc@a63
PS1, Line 63: 
> Not sure why, but I got errors like this if I don't remove this line.
Looks the new version thrift would generate to_string functions in the headers while the old doesn't have them.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 17:02:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 1:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/10849/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 15:53:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/runtime/io/disk-io-mgr.cc@a63
PS1, Line 63: 
> Is the change necessary for this patch?
Not sure why, but I got errors like this if I don't remove this line.

/home/systest/workspace/impala/be/generated-sources/gen-cpp/CatalogObjects_types.h:198:13: note: candidate: std::__cxx11::string impala::to_string(const imp
ala::TTablePropertyType::type&) <near match>
 std::string to_string(const TTablePropertyType::type& val);
             ^~~~~~~~~
/home/systest/workspace/impala/be/generated-sources/gen-cpp/CatalogObjects_types.h:198:13: note:   conversion of argument 1 would be ill-formed:
/home/systest/workspace/impala/be/src/runtime/io/disk-io-mgr.cc:505:77: error: invalid conversion from ‘int’ to ‘impala::TTablePropertyType::type’ [-fpermis
sive]
           i < DiskInfo::num_disks() ? DiskInfo::device_name(i) : to_string(i);
                                                                             ^
In file included from /home/systest/workspace/impala/be/src/util/error-util.h:26:0,
                 from /home/systest/workspace/impala/be/src/common/status.h:31,
                 from /home/systest/workspace/impala/be/src/runtime/io/disk-io-mgr.h:29,
                 from /home/systest/workspace/impala/be/src/runtime/io/disk-io-mgr.cc:18:



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 15:51:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18661/2/be/src/rpc/thrift-util.cc
File be/src/rpc/thrift-util.cc:

http://gerrit.cloudera.org:8080/#/c/18661/2/be/src/rpc/thrift-util.cc@164
PS2, Line 164: bool IsReadTimeoutTException(const TTransportException& e) {
             :   // String taken from TSocket::read() Thrift's TSocket.cpp and TSSLSocket.cpp.
             :   return (e.getType() == TTransportException::TIMED_OUT &&
             :              strstr(e.what(), "EAGAIN (timed out)") != nullptr) ||
             :          (e.getType() == TTransportException::INTERNAL_ERROR &&
             :              strstr(e.what(), "SSL_read: Resource temporarily unavailable") != nullptr);
             : }
             : 
             : bool IsPeekTimeoutTException(const TTransportException& e) {
             :   // String taken from TSocket::peek() Thrift's TSocket.cpp and TSSLSocket.cpp.
             :   return (e.getType() == TTransportException::UNKNOWN &&
             :              strstr(e.what(), "recv(): Resource temporarily unavailable") != nullptr) ||
             :          (e.getType() == TTransportException::INTERNAL_ERROR &&
             :              strstr(e.what(), "SSL_peek: Resource temporarily unavailable") != nullptr);
             : }
             : 
             : bool IsConnResetTException(const TTransportException& e) {
             :   // Strings taken from TTransport::readAll(). This happens iff TSocket::read() returns 0.
             :   // As readAll() is reading non-zero length payload, this can only mean recv() called
             :   // by read() returns 0. According to man page of recv(), this implies a stream socket
             :   // peer has performed an orderly shutdown.
             :   return (e.getType() == TTransportException::END_OF_FILE &&
             :              strstr(e.what(), "No more data to read.") != nullptr) ||
             :          (e.getType() == TTransportException::INTERNAL_ERROR &&
             :              strstr(e.what(), "SSL_read: Connection reset by peer") != nullptr);
             : }
> These exceptions are one thing that can change with newer Thrift versions. 
I look around and it does not seem that we need to change this.
The only failing test after upgrade is TestGracefulShutdown.test_shutdown_idle, and it is addressed by change in client-request-state.cc.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 20:29:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 4:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/10857/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 21:30:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 24 Jun 2022 23:01:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/runtime/io/disk-io-mgr.cc@a63
PS1, Line 63: 
Is the change necessary for this patch?


http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/util/process-state-info.cc
File be/src/util/process-state-info.cc:

http://gerrit.cloudera.org:8080/#/c/18661/1/be/src/util/process-state-info.cc@a40
PS1, Line 40: 
Same with the other comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 15:42:39 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 3:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/10856/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 20:45:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................

IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

This patch upgrades IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 to mitigate
CVE-2020-13949 and hang issue with newer JDBC client.
IMPALA_TOOLCHAIN_BUILD_ID is upgraded to 179-9977806f06, which contains
the required thrift-0.16.0 compiler.

The refactoring itself includes:
- Removing non-generated (empty) *_constants.cpp and adjustment for
  THRIFT-4730.
- Adjusts error handling in client-request-state.cc due to changing
  thrift error message after the upgrade.
- Adding custom build target thrift-cpp-manual-edit to fix
  clang-diagnostic-inconsistent-missing-override warning in generated
  ImpalaHiveServer2Service.h and ImpalaService.h

Testing:
- Build and pass core tests.

Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/service/client-request-state.cc
M be/src/transport/THttpTransport.cpp
M be/src/transport/THttpTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/transport/TSaslTransport.h
M be/src/util/process-state-info.cc
M bin/impala-config.sh
M common/thrift/CMakeLists.txt
15 files changed, 38 insertions(+), 60 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8269/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 24 Jun 2022 17:55:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18661/2/be/src/rpc/thrift-util.cc
File be/src/rpc/thrift-util.cc:

http://gerrit.cloudera.org:8080/#/c/18661/2/be/src/rpc/thrift-util.cc@164
PS2, Line 164: bool IsReadTimeoutTException(const TTransportException& e) {
             :   // String taken from TSocket::read() Thrift's TSocket.cpp and TSSLSocket.cpp.
             :   return (e.getType() == TTransportException::TIMED_OUT &&
             :              strstr(e.what(), "EAGAIN (timed out)") != nullptr) ||
             :          (e.getType() == TTransportException::INTERNAL_ERROR &&
             :              strstr(e.what(), "SSL_read: Resource temporarily unavailable") != nullptr);
             : }
             : 
             : bool IsPeekTimeoutTException(const TTransportException& e) {
             :   // String taken from TSocket::peek() Thrift's TSocket.cpp and TSSLSocket.cpp.
             :   return (e.getType() == TTransportException::UNKNOWN &&
             :              strstr(e.what(), "recv(): Resource temporarily unavailable") != nullptr) ||
             :          (e.getType() == TTransportException::INTERNAL_ERROR &&
             :              strstr(e.what(), "SSL_peek: Resource temporarily unavailable") != nullptr);
             : }
             : 
             : bool IsConnResetTException(const TTransportException& e) {
             :   // Strings taken from TTransport::readAll(). This happens iff TSocket::read() returns 0.
             :   // As readAll() is reading non-zero length payload, this can only mean recv() called
             :   // by read() returns 0. According to man page of recv(), this implies a stream socket
             :   // peer has performed an orderly shutdown.
             :   return (e.getType() == TTransportException::END_OF_FILE &&
             :              strstr(e.what(), "No more data to read.") != nullptr) ||
             :          (e.getType() == TTransportException::INTERNAL_ERROR &&
             :              strstr(e.what(), "SSL_read: Connection reset by peer") != nullptr);
             : }
These exceptions are one thing that can change with newer Thrift versions. If you haven't already, can you check the Thrift source to see if the messages match or if any other messages should fall under these categories?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 17:33:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 2:

> Patch Set 2:
> 
> Build Failed 
> 
> https://jenkins.impala.io/job/gerrit-code-review-checks/10851/ : Initial code review checks failed. See linked job for details on the failure.

clang tidy complains about missing override keyword.
I can fix it manually using sed against generated code like this:

sed -i 's/\(dispatchCallTemplated.*\)callContext)/\1callContext) override/' ImpalaHiveServer2Service.h

However, I haven't find a good way to hook it up in common/thrift/CMakeLists.txt.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 18:26:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10860/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 23 Jun 2022 21:59:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 24 Jun 2022 22:38:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18661 )

Change subject: IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0
......................................................................

IMPALA-11384: Upgrade CPP thrift components to thrift-0.16.0

This patch upgrades IMPALA_THRIFT_CPP_VERSION=0.16.0-p3 to mitigate
CVE-2020-13949 and hang issue with newer JDBC client.
IMPALA_TOOLCHAIN_BUILD_ID is upgraded to 179-9977806f06, which contains
the required thrift-0.16.0 compiler.

The refactoring itself includes:
- Removing non-generated (empty) *_constants.cpp and adjustment for
  THRIFT-4730.
- Adjusts error handling in client-request-state.cc due to changing
  thrift error message after the upgrade.
- Adding custom build target thrift-cpp-manual-edit to fix
  clang-diagnostic-inconsistent-missing-override warning in generated
  ImpalaHiveServer2Service.h and ImpalaService.h

Testing:
- Build and pass core tests.

Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Reviewed-on: http://gerrit.cloudera.org:8080/18661
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
---
M be/generated-sources/gen-cpp/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/service/client-request-state.cc
M be/src/transport/THttpTransport.cpp
M be/src/transport/THttpTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/transport/TSaslTransport.h
M be/src/util/process-state-info.cc
M bin/impala-config.sh
M common/thrift/CMakeLists.txt
15 files changed, 38 insertions(+), 60 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Joe McDonnell: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic278ac5c973ff5c3e829a6139b9c16e9d2c62a59
Gerrit-Change-Number: 18661
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>