[Discuss] Pulsar client : Connect Command add keep_alive_interval

[Technoboy-]

Motivation

When investigating #13342, we found that both the client and the server have the keepAliveIntervalSeconds configuration, which is 30s by default. During the configured time, the channel will send ping/pong commands to maintain connection availability. If the pong command is not replied within the cycle, the channel will be closed. For the client side, the reconnect logic is triggered after the channel is closed. For the broker side, the broker will clear the producer information after the channel is inactive.
For the problem of #13342, it is because the user changed the configuration on the broker side to 100s. When the client determines that the connection has timed out and needs to disconnect the channel, since the client to the broker passes through the firewall, the close of the channel may not be sent, and then the client reconnects to the broker, and the reconnection succeeds. However, the timeout setting of the broker is relatively large. If the previous channel is not closed, the producer information will not be cleared. The reconnection of the producer will cause the broker to throw the exception that the producer already exists.
This is the cause of the #13342 issue, and by tweaking the code, the issue can be reproduced.

What I want to discuss is whether we can optimize this, configure this value only on the client-side, and pass it to the broker through the connect command. The advantage is that the server can cancel this configuration, using client-side value instead, and multiple clients can configure different values.

API Changes

Add keep_alive_interval in CommandConnect:

message CommandConnect {
     ...
     optional int32 keep_alive_interval = 11 [default = 30];
}

The original logic to check the keep-alive is in PulsarHandler#handleKeepAliveTimeout which begins at channel active.

For broker-side:

• Now we do this in ServerCnx#completeConnect

For client-side:

• Now we do this in ClientCnx#handleConnected

Compatibility

no compatibility issues

[lhotari]

What I want to discuss is whether we can optimize this, configure this value only on the client-side, and pass it to the broker through the connect command. The advantage is that the server can cancel this configuration, and multiple clients can configure different values.

I think that this is unnecessary complexity. The broker side keep alive should be kept short enough and that should be independent of the client setting.

There were some bugs that caused the connection to stay open for longs durations.
#15382 Closes the connection if a ping or pong message cannot be sent .
#15366 Fixes a proxy connection leak when inbound connection closes while connecting is in progress
Both of the above fixes address issues where the symptom is "Producer with name xxx is already connect to topic".

I don't think that we need this PIP at all.

[lhotari]

The steps for reproducing "Producer with name xxx is already connect to topic" in #13342 doesn't describe a bug. It's expected and correct behavior of the broker. I commented in #13342 (comment) about this. Therefore this PIP is attempting to solve a problem that isn't a problem at all.

[Technoboy-]

The steps for reproducing "Producer with name xxx is already connect to topic" in #13342 doesn't describe a bug. It's expected and correct behavior of the broker. I commented in #13342 (comment) about this. Therefore this PIP is attempting to solve a problem that isn't a problem at all.

I don't think it's a problem either, only an optimization. The broker side does not need this configuration at all, and uses the client-side instead.

[lhotari]

I don't think it's a problem either, only an optimization. The broker side does not need this configuration at all, and uses the client-side instead.

I don't see how this could be an optimization when it's not needed at all.

[Technoboy-]

I don't think it's a problem either, only an optimization. The broker side does not need this configuration at all, and uses the client-side instead.

I don't see how this could be an optimization when it's not needed at all.

Broker doesn't need this configuration anymore, that's not an optimization?

[lhotari]

I don't think it's a problem either, only an optimization. The broker side does not need this configuration at all, and uses the client-side instead.

I don't see how this could be an optimization when it's not needed at all.

Broker doesn't need this configuration anymore, that's not an optimization?

There is no need to make the broker's keep alive interval match the client's keep alive interval. They should be kept separate.

[Technoboy-]

I don't think it's a problem either, only an optimization. The broker side does not need this configuration at all, and uses the client-side instead.

I don't see how this could be an optimization when it's not needed at all.

Broker doesn't need this configuration anymore, that's not an optimization?

There is no need to make the broker's keep alive interval match the client's keep alive interval. They should be kept separate.

Why should be separated?

[Technoboy-]

I don't think it's a problem either, only an optimization. The broker side does not need this configuration at all, and uses the client-side instead.

I don't see how this could be an optimization when it's not needed at all.

Broker doesn't need this configuration anymore, that's not an optimization?

There is no need to make the broker's keep alive interval match the client's keep alive interval. They should be kept separate.

This value is based on the channel according to the client configuration, is there any problem? Why separate configuration?

[lhotari]

Why should be separated?
This value is based on the channel according to the client configuration, is there any problem? Why separate configuration?

Since there isn't any problem that needs to be solved. Making the broker keep alive interval match the client's keep alive interval isn't solving a real problem. The broker side keep alive interval should be kept at 30 seconds and there's nothing to change.
We could simply document this behavior that if you increase the keep alive interval, there's a risk that connections are orphaned until the keep alive check runs.

[github-actions]

The issue had no activity for 30 days, mark with Stale label.