[fix][websocket] Fix webSocketPingDurationSeconds config #19256
Conversation
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
|
/pulsarbot rerun-failure-checks |
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19256 +/- ##
============================================
+ Coverage 47.04% 52.81% +5.77%
- Complexity 9190 22326 +13136
============================================
Files 607 1824 +1217
Lines 57677 136681 +79004
Branches 6007 15037 +9030
============================================
+ Hits 27132 72194 +45062
- Misses 27598 56975 +29377
- Partials 2947 7512 +4565
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Ping @poorbarcode |
| category = CATEGORY_WEBSOCKET, | ||
| doc = "Interval of time to sending the ping to keep alive in WebSocket proxy. " | ||
| + "This value greater than 0 means enabled") | ||
| private int webSocketPingDurationSeconds = -1; |
There was a problem hiding this comment.
It is better to use webSocketKeepAliveIntervalSeconds to make it consistent with the existing name keepAliveIntervalSeconds
And we should also add keepAliveIntervalSeconds to websocket.conf
There was a problem hiding this comment.
We already add this to other branches, so I don't suggest rename to keepAliveIntervalSeconds.
| @@ -192,18 +191,15 @@ private void closePingFuture() { | |||
| @Override | |||
| public void onWebSocketConnect(Session session) { | |||
| super.onWebSocketConnect(session); | |||
| WebSocketProxyConfiguration webSocketProxyConfig = service.getWebSocketProxyConfig(); | |||
| if (webSocketProxyConfig != null) { | |||
| int webSocketPingDurationSeconds = webSocketProxyConfig.getWebSocketPingDurationSeconds(); | |||
There was a problem hiding this comment.
Does the field webSocketPingDurationSeconds in class WebSocketProxyConfiguration not useful now? If yes, should we delete it?
There was a problem hiding this comment.
No, these configurations can be converted to each other, so add webSocketPingDurationSeconds to each config.
There was a problem hiding this comment.
There was a problem hiding this comment.
No. My changes don't break the tests of #19203.
There was a problem hiding this comment.
No. My changes don't break the tests of #19203.
Ah, yes. you are right. The configuration that acts on WebSocketProxyConfiguration is copied to ServiceConfiguration.
|
branch-2.10 and branch-2.11 include these changes. |
Motivation
#19203 introduces
webSocketPingDurationSecondsconfig, but doesn't add this config to the proxy and broker config class.When the proxy/broker calls the
WebSocketService(ClusterData localCluster, ServiceConfiguration config), the ping feature will be broken.Modifications
webSocketPingDurationSecondstoProxyConfigurationwebSocketPingDurationSecondstoServiceConfigurationwebSocketPingDurationSecondsDocumentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: