scwhittle commented on code in PR #36528:
URL: https://github.com/apache/beam/pull/36528#discussion_r2449359641


##########
sdks/python/apache_beam/runners/worker/channel_factory.py:
##########
@@ -23,8 +23,24 @@
 
 class GRPCChannelFactory(grpc.StreamStreamClientInterceptor):
   DEFAULT_OPTIONS = [
-      ("grpc.keepalive_time_ms", 20000),
-      ("grpc.keepalive_timeout_ms", 300000),
+      # Default: 30000ms (30s), increased to 180s to reduce ping frequency
+      ("grpc.keepalive_time_ms", 180000),
+      # Default: 5000ms (5s), increased to 10 minutes for stability
+      ("grpc.keepalive_timeout_ms", 600000),
+      # Default: 2, set to 0 to allow unlimited pings without data
+      ("grpc.http2.max_pings_without_data", 0),
+      # Default: False, set to True to allow keepalive pings when no calls
+      ("grpc.keepalive_permit_without_calls", True),
+      # Default: 300000ms (5min), increased to 10min for stability
+      ("grpc.http2.min_recv_ping_interval_without_data_ms", 600000),
+      # Default: 300000ms (5min), increased to 120s for conservative pings
+      ("grpc.http2.min_sent_ping_interval_without_data_ms", 120000),

Review Comment:
   I don't see this documented, I believe it has been deprecated
   



##########
sdks/python/apache_beam/runners/worker/channel_factory.py:
##########
@@ -23,8 +23,24 @@
 
 class GRPCChannelFactory(grpc.StreamStreamClientInterceptor):
   DEFAULT_OPTIONS = [
-      ("grpc.keepalive_time_ms", 20000),
-      ("grpc.keepalive_timeout_ms", 300000),
+      # Default: 30000ms (30s), increased to 180s to reduce ping frequency

Review Comment:
   Are the pings expensive if we only have a single channel? Does this mean 
that it will take 180s instead of 30s to notice that the runner terminated?  
This doesn't seem good for streaming pipelines if the case.



##########
sdks/python/apache_beam/runners/worker/channel_factory.py:
##########
@@ -23,8 +23,24 @@
 
 class GRPCChannelFactory(grpc.StreamStreamClientInterceptor):
   DEFAULT_OPTIONS = [
-      ("grpc.keepalive_time_ms", 20000),
-      ("grpc.keepalive_timeout_ms", 300000),
+      # Default: 30000ms (30s), increased to 180s to reduce ping frequency
+      ("grpc.keepalive_time_ms", 180000),
+      # Default: 5000ms (5s), increased to 10 minutes for stability
+      ("grpc.keepalive_timeout_ms", 600000),
+      # Default: 2, set to 0 to allow unlimited pings without data
+      ("grpc.http2.max_pings_without_data", 0),
+      # Default: False, set to True to allow keepalive pings when no calls
+      ("grpc.keepalive_permit_without_calls", True),
+      # Default: 300000ms (5min), increased to 10min for stability
+      ("grpc.http2.min_recv_ping_interval_without_data_ms", 600000),
+      # Default: 300000ms (5min), increased to 120s for conservative pings
+      ("grpc.http2.min_sent_ping_interval_without_data_ms", 120000),
+      # Default: 2, set to 0 to allow unlimited ping strikes
+      ("grpc.http2.max_ping_strikes", 0),

Review Comment:
   think this just affects server, rm?
   
   https://github.com/grpc/grpc/blob/master/doc/keepalive.md



##########
sdks/python/apache_beam/runners/worker/channel_factory.py:
##########
@@ -23,8 +23,24 @@
 
 class GRPCChannelFactory(grpc.StreamStreamClientInterceptor):
   DEFAULT_OPTIONS = [
-      ("grpc.keepalive_time_ms", 20000),
-      ("grpc.keepalive_timeout_ms", 300000),
+      # Default: 30000ms (30s), increased to 180s to reduce ping frequency
+      ("grpc.keepalive_time_ms", 180000),
+      # Default: 5000ms (5s), increased to 10 minutes for stability
+      ("grpc.keepalive_timeout_ms", 600000),
+      # Default: 2, set to 0 to allow unlimited pings without data
+      ("grpc.http2.max_pings_without_data", 0),
+      # Default: False, set to True to allow keepalive pings when no calls
+      ("grpc.keepalive_permit_without_calls", True),
+      # Default: 300000ms (5min), increased to 10min for stability
+      ("grpc.http2.min_recv_ping_interval_without_data_ms", 600000),
+      # Default: 300000ms (5min), increased to 120s for conservative pings
+      ("grpc.http2.min_sent_ping_interval_without_data_ms", 120000),
+      # Default: 2, set to 0 to allow unlimited ping strikes
+      ("grpc.http2.max_ping_strikes", 0),
+      # Default: 0 (disabled), enable socket reuse for better handling
+      ("grpc.so_reuseport", 1),
+      # Default: 0 (disabled), set 30s TCP timeout for connection control
+      ("grpc.tcp_user_timeout_ms", 30000),

Review Comment:
   does python use c-core for grpc? if so it seems that this does default to 
20s not 0
   
   where are you seeing the defaults?



##########
sdks/python/apache_beam/runners/worker/channel_factory.py:
##########
@@ -23,8 +23,24 @@
 
 class GRPCChannelFactory(grpc.StreamStreamClientInterceptor):
   DEFAULT_OPTIONS = [
-      ("grpc.keepalive_time_ms", 20000),
-      ("grpc.keepalive_timeout_ms", 300000),
+      # Default: 30000ms (30s), increased to 180s to reduce ping frequency
+      ("grpc.keepalive_time_ms", 180000),
+      # Default: 5000ms (5s), increased to 10 minutes for stability
+      ("grpc.keepalive_timeout_ms", 600000),
+      # Default: 2, set to 0 to allow unlimited pings without data
+      ("grpc.http2.max_pings_without_data", 0),
+      # Default: False, set to True to allow keepalive pings when no calls
+      ("grpc.keepalive_permit_without_calls", True),
+      # Default: 300000ms (5min), increased to 10min for stability
+      ("grpc.http2.min_recv_ping_interval_without_data_ms", 600000),

Review Comment:
   think affects server only 
https://github.com/grpc/grpc/blob/master/doc/keepalive.md



##########
sdks/python/apache_beam/runners/worker/channel_factory.py:
##########
@@ -23,8 +23,24 @@
 
 class GRPCChannelFactory(grpc.StreamStreamClientInterceptor):
   DEFAULT_OPTIONS = [
-      ("grpc.keepalive_time_ms", 20000),
-      ("grpc.keepalive_timeout_ms", 300000),
+      # Default: 30000ms (30s), increased to 180s to reduce ping frequency
+      ("grpc.keepalive_time_ms", 180000),
+      # Default: 5000ms (5s), increased to 10 minutes for stability
+      ("grpc.keepalive_timeout_ms", 600000),
+      # Default: 2, set to 0 to allow unlimited pings without data
+      ("grpc.http2.max_pings_without_data", 0),
+      # Default: False, set to True to allow keepalive pings when no calls
+      ("grpc.keepalive_permit_without_calls", True),
+      # Default: 300000ms (5min), increased to 10min for stability
+      ("grpc.http2.min_recv_ping_interval_without_data_ms", 600000),
+      # Default: 300000ms (5min), increased to 120s for conservative pings
+      ("grpc.http2.min_sent_ping_interval_without_data_ms", 120000),
+      # Default: 2, set to 0 to allow unlimited ping strikes
+      ("grpc.http2.max_ping_strikes", 0),
+      # Default: 0 (disabled), enable socket reuse for better handling
+      ("grpc.so_reuseport", 1),

Review Comment:
   also server only?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to