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 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/21420/2/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: http://gerrit.cloudera.org:8080/#/c/21420/2/be/src/rpc/thrift-util.cc@94 PS2, Line 94: : // The ThriftSerializer uses the DefaultInternalTConfiguration() with the higher limit, : // because this is used on our internal Thrift structures. : ThriftSerializer::ThriftSerializer(bool compact, int initial_buffer_size) > I thought of increasing it to over than 2GB. But thinking again, it is prob I tried this out, and it was interesting. 1) It uses a lot of memory (10s of GBs). 2) It turns out that it can't really work. We can serialize a bigger message (and the max message size is not enforced by the serializer, so the limit passed in is irrelevant). However, the deserializer uses the DefaultExternalTConfiguration() with a 2GB limit and hits the max message size when deserializing the message we created. The deserializer is used on untrusted data, so we don't really want it to have the higher limit. So, I've left this as-is. -- 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: 2 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Fri, 17 May 2024 05:53:25 +0000 Gerrit-HasComments: Yes
