Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/24786 )
Change subject: IMPALA-15257: Add TCP keepalive support for internal Thrift traffic ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/24786/4/be/src/rpc/thrift-util.h File be/src/rpc/thrift-util.h: http://gerrit.cloudera.org:8080/#/c/24786/4/be/src/rpc/thrift-util.h@37 PS4, Line 37: DECLARE_int32(internal_keepalive_probe_period_s); I think it's a little unusual for us to add declarations to a header file. I do see other examples though. http://gerrit.cloudera.org:8080/#/c/24786/4/tests/custom_cluster/test_internal_keepalive.py File tests/custom_cluster/test_internal_keepalive.py: http://gerrit.cloudera.org:8080/#/c/24786/4/tests/custom_cluster/test_internal_keepalive.py@34 PS4, Line 34: SS_POLL_ATTEMPTS = 20 Might be worth a run with asan to make sure this is long enough for slow test environments. http://gerrit.cloudera.org:8080/#/c/24786/4/tests/custom_cluster/test_internal_keepalive.py@41 PS4, Line 41: Thrift connections. The impalad<->statestored connection (port24000) is a A space between port24000 might be nice. http://gerrit.cloudera.org:8080/#/c/24786/4/tests/custom_cluster/test_internal_keepalive.py@73 PS4, Line 73: # The disabled case must be quite across the whole window. quite -> quiet? -- To view, visit http://gerrit.cloudera.org:8080/24786 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9fec7a02b2c5ef92e8b68de9e3993bcee7bc898 Gerrit-Change-Number: 24786 Gerrit-PatchSet: 4 Gerrit-Owner: Jiyoung Yoo <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 03 Sep 2026 18:42:28 +0000 Gerrit-HasComments: Yes
