[refactor][proxy] Refactor Proxy code and fix connection stalling by switching to auto read mode by lhotari · Pull Request #14713 · apache/pulsar

lhotari · 2022-03-16T09:11:50Z

Motivation

Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete.

Currently, the proxy calls .read() when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode.

Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker:

pulsar/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

Line 112 in a269053

    
           b.group(inboundChannel.eventLoop()).channel(inboundChannel.getClass()).option(ChannelOption.AUTO_READ, false);

Modifications

replace broker host parsing with a simple solution
pass remote host name to ProxyBackendHandler in the constructor
rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string
move HA proxy message handling to ProxyBackendHandle and extract the logic to a method
remove the static "inboundOutboundChannelMap" which was used for log level 2
- make it obsolete by passing the peer channel id to ParserProxyHandler
Enable auto read in proxy and remove ctx.read() / channel.read() calls
prepare for IPv6 support (reported as IPv6 Support for broker and proxy with bindAddress #14732) by improving the host:port parsing (pick last : since IPv6 address might contains multiple : characters)
Handle backpressure properly by switching auto read off when channel writability changes
- change auto read of the proxy-broker connection based on the writability of the client-proxy connection
- change auto read of the client-proxy connection based on the writability of the proxy-broker connection
Consistently handle write errors by delegating exception handling to exceptionCaught method by using .addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)

michaeljmarshall

LGTM

eolivelli

nice and useful work !

eolivelli · 2022-03-17T09:02:26Z

@lhotari please resolve the conflicts

…exOf(':')

lhotari · 2022-03-17T16:09:29Z

I ended up increasing the scope of this PR and added the fix to a connection stalling issue. Please review again @eolivelli @codelipenghui @michaeljmarshall @nicoloboschi @merlimat

eolivelli

+1

lhotari · 2022-03-17T16:32:33Z

I asked for feedback in the community meeting from @merlimat . Based on the discussion there was the conclusion that backpressure is needed to prevent a situation where the proxy buffers grow in the case where either the broker connection is slow or the client connection is slow.

…ity changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection

lhotari · 2022-03-17T17:09:57Z

@merlimat I have implemented the solution for flow control in a04aa5f . Please review

lhotari · 2022-03-17T17:19:05Z

The channel writability change feature in Netty can be configured with setWriteBufferWaterMark or setWriteBufferLowWaterMark/setWriteBufferHighWaterMark methods. By default, the low water mark is 32kB and high water mark is 64kB. That sounds like a sensible default.

- delegate exception handling to exceptionCaught method

michaeljmarshall

LGTM

…switching to auto read mode (apache#14713) Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as apache#14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)` (cherry picked from commit a1037c7)

…switching to auto read mode (apache#14713) ### Motivation Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 ### Modifications - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as apache#14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)` (cherry picked from commit a1037c7) # Conflicts: # pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java # pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ParserProxyHandler.java # pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java

…switching to auto read mode (#14713) ### Motivation Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 ### Modifications - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as #14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)` (cherry picked from commit a1037c7)

…switching to auto read mode (#14713) Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as #14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)` (cherry picked from commit a1037c7)

…switching to auto read mode (apache#14713) ### Motivation Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 ### Modifications - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as apache#14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)`

…switching to auto read mode (apache#14713) Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as apache#14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)` (cherry picked from commit a1037c7) (cherry picked from commit 1e55685)

…switching to auto read mode (#14713) Refactor Proxy code to make it easier to understand and maintain. In addition, switch to use auto read mode since the proxies connections seem to stall in some cases since the proxied connection doesn't use Netty's auto read mode and the read handling doesn't seem complete. Currently, the proxy calls `.read()` when a message is written to the connection. There might be more messages flowing in the other direction and it could result in a blocked connection with the current solution that doesn't use Netty's auto read mode. Currently auto read is disabled in DirectProxyHandler for the connection between the proxy and the broker: https://github.com/apache/pulsar/blob/a26905371749798ec5288fb07a69978a36aacfaa/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java#L112 - replace broker host parsing with a simple solution - pass remote host name to ProxyBackendHandler in the constructor - rename "targetBrokerUrl" to "brokerHostAndPort" since the "targetBrokerUrl" is really "hostname:port" string - move HA proxy message handling to ProxyBackendHandle and extract the logic to a method - remove the static "inboundOutboundChannelMap" which was used for log level 2 - make it obsolete by passing the peer channel id to ParserProxyHandler - Enable auto read in proxy and remove `ctx.read()` / `channel.read()` calls - prepare for IPv6 support (reported as #14732) by improving the `host:port` parsing (pick last `:` since IPv6 address might contains multiple `:` characters) - Handle backpressure properly by switching auto read off when channel writability changes - change auto read of the proxy-broker connection based on the writability of the client-proxy connection - change auto read of the client-proxy connection based on the writability of the proxy-broker connection - Consistently handle write errors by delegating exception handling to exceptionCaught method by using `.addListener(ChannelFutureListener.FIRE_EXCEPTION_ON_FAILURE)` (cherry picked from commit a1037c7) (cherry picked from commit 1e55685) (cherry picked from commit 9673d6c)

lhotari added area/proxy doc-not-needed Your PR changes do not impact docs labels Mar 16, 2022

lhotari requested review from addisonj, codelipenghui, dlg99, eolivelli, merlimat and michaeljmarshall March 16, 2022 09:11

lhotari self-assigned this Mar 16, 2022

nicoloboschi approved these changes Mar 16, 2022

View reviewed changes

michaeljmarshall approved these changes Mar 16, 2022

View reviewed changes

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

michaeljmarshall added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label Mar 16, 2022

michaeljmarshall added this to the 2.11.0 milestone Mar 16, 2022

eolivelli approved these changes Mar 16, 2022

View reviewed changes

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

codelipenghui approved these changes Mar 17, 2022

View reviewed changes

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java

lhotari added 6 commits March 17, 2022 12:27

[Proxy] Refactor proxy code to simplify and clarify it

59e6ffc

Make parsing of broker host and port IPv6 compatible by using lastInd…

09a80ee

…exOf(':')

Use thenAcceptAsync so that exceptions are handled

11346c5

Enable auto read in proxy and remove ctx.read() calls

72974ad

Fix comment

ca6e8df

Refactor future listeners

4a24061

lhotari force-pushed the lh-refactor-proxy-code branch from 3c3c40e to 4a24061 Compare March 17, 2022 11:22

lhotari marked this pull request as draft March 17, 2022 11:53

lhotari changed the title ~~[Proxy] Refactor Proxy code~~ [Proxy] Refactor Proxy code and fix connection stalling by switching to auto read mode Mar 17, 2022

lhotari marked this pull request as ready for review March 17, 2022 16:06

lhotari requested review from codelipenghui, eolivelli and michaeljmarshall March 17, 2022 16:08

eolivelli approved these changes Mar 17, 2022

View reviewed changes

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/DirectProxyHandler.java Outdated

lhotari marked this pull request as draft March 17, 2022 16:31

lhotari marked this pull request as ready for review March 17, 2022 17:09

Address review feedback: make utility method static

8a08755

lhotari added the type/bug The PR fixed a bug or issue reported a bug label Mar 18, 2022

Consistently handle write errors

5dfdf12

- delegate exception handling to exceptionCaught method

michaeljmarshall approved these changes Mar 21, 2022

View reviewed changes

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java

codelipenghui approved these changes Mar 22, 2022

View reviewed changes

codelipenghui merged commit a1037c7 into apache:master Mar 22, 2022

codelipenghui changed the title ~~[Proxy] Refactor Proxy code and fix connection stalling by switching to auto read mode~~ [refactor][proxy] Refactor Proxy code and fix connection stalling by switching to auto read mode Mar 22, 2022

lhotari mentioned this pull request Mar 24, 2022

[Proxy] Pulsar Handshake was not completed within timeout, Proxy stops working for proxying Broker connections while Admin API proxying keeps working , #14831

Open

lhotari added release/2.9.3 release/2.8.4 release/2.10.1 labels Mar 24, 2022

lhotari added the cherry-picked/branch-2.10 label Mar 24, 2022

lhotari added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life labels Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[refactor][proxy] Refactor Proxy code and fix connection stalling by switching to auto read mode - #14713

[refactor][proxy] Refactor Proxy code and fix connection stalling by switching to auto read mode#14713
codelipenghui merged 9 commits into
apache:masterfrom
lhotari:lh-refactor-proxy-code

lhotari commented Mar 16, 2022 •

edited

Loading

Uh oh!

michaeljmarshall left a comment

Uh oh!

Uh oh!

eolivelli left a comment

Uh oh!

Uh oh!

eolivelli commented Mar 17, 2022

Uh oh!

Uh oh!

lhotari commented Mar 17, 2022 •

edited

Loading

Uh oh!

eolivelli left a comment

Uh oh!

Uh oh!

lhotari commented Mar 17, 2022

Uh oh!

lhotari commented Mar 17, 2022

Uh oh!

lhotari commented Mar 17, 2022 •

edited

Loading

Uh oh!

michaeljmarshall left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

lhotari commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eolivelli commented Mar 17, 2022

Uh oh!

Uh oh!

lhotari commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhotari commented Mar 17, 2022

Uh oh!

lhotari commented Mar 17, 2022

Uh oh!

lhotari commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lhotari commented Mar 16, 2022 •

edited

Loading

lhotari commented Mar 17, 2022 •

edited

Loading

lhotari commented Mar 17, 2022 •

edited

Loading