Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21420 )
Change subject: IMPALA-13020 (part 2): Split out external vs internal Thrift max message size ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/catalog/catalogd-main.cc File be/src/catalog/catalogd-main.cc: http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/catalog/catalogd-main.cc@87 PS1, Line 87: // Mark this as an internal service to use a more permission Thrift max message size > nit: permissive Done http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/common/init.cc File be/src/common/init.cc: http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/common/init.cc@548 PS1, Line 548: FLAGS_thrift_external_rpc_max_message_size < ThriftDefaultMaxMessageSize() > Is there a possibility it would parse as unsigned but then get used as sign It's defined as an int64 gflag, so gflags is parsing it with the right data type and I see negative values getting passed through properly. If I try to specify a value outside that range, I get: ERROR: illegal value '-9223372036854775809' specified for int64 flag 'thrift_rpc_max_message_size' In testing that, I noticed that 0/negative doesn't work, because the frontend isn't converting it to the Thrift default. I fixed that. I added some text to the flag description saying that it needs to be at least as big as the Thrift default value and changed this error message to try to make it clearer. The new message doesn't mention setting <= 0 as I think that just makes it harder to understand. I think I'm going to skip having an upper limit for now. In theory, we can bound this by the amount of memory on the system. http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/rpc/authentication.cc File be/src/rpc/authentication.cc: http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/rpc/authentication.cc@1350 PS1, Line 1350: ThriftExternalRpcMaxMessageSize() == max_message_size || : ThriftInternalRpcMaxMessageSize() == max_message_size > This DCHECK is repeated in three separate places. Maybe consider making an Changed this into a function that verifies the max message size has been inherited properly. It combines this check with the check that the wrapped transport has the same limit. http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/rpc/thrift-server.cc File be/src/rpc/thrift-server.cc: http://gerrit.cloudera.org:8080/#/c/21420/1/be/src/rpc/thrift-server.cc@288 PS1, Line 288: is_external_facing_(is_external_facing) > If is_external_facing_ == false, can we DCHECK if it is legal given specifi I coded this up and ended up not liking it very much. ThriftServer is a library. In order for it to have a list of valid internal services, we need to break the boundary and have it know about statestore, catalog, etc. It is very rare for us to create a new Thrift service and the default is to be external facing, so I'd rather not add that extra check. -- To view, visit http://gerrit.cloudera.org:8080/21420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9a649ef49a8a99c7bd9a1b73c37c4c621661311 Gerrit-Change-Number: 21420 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 16 May 2024 00:38:48 +0000 Gerrit-HasComments: Yes
