poorbarcode commented on code in PR #20568:
URL: https://github.com/apache/pulsar/pull/20568#discussion_r1243589832
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -3417,6 +3429,17 @@ public CompletableFuture<Boolean>
checkConnectionLiveness() {
}
}
+ @Override
+ public void healthCheckManually() {
+ long lastCheckTime =
LAST_MANUAL_HEARTBEAT_CHECK_TIME_UPDATER.get(this);
+ if (System.currentTimeMillis() - lastCheckTime < 5000) {
+ return;
+ }
+ if (LAST_MANUAL_HEARTBEAT_CHECK_TIME_UPDATER.compareAndSet(this,
lastCheckTime, System.currentTimeMillis())) {
+ sendPing();
Review Comment:
> Is it better to change the log level to debug? It's a message level log,
for a cluster with high consumption throughput, it will introduce too many
warning logs. We should be careful to add logs on the message level with >=
info level.
Fixed.
> Is it possible to add a test to ensure the connection will be closed
immediately if failed to write data to the channel?
After I wrote a test, I noticed that the logic of "close connection after
the write failure" already exists<sup>[1]</sup>, I removed the logic which
tries to do the same thing. Now the modification of this PR is the following
two points:
- **(important)** Since `cnx.address + consumerId` is the identifier of one
consumer; add `consumer-id` into the log when doing subscribe.
- add a test to confirm that even if the error occurs when sending messages
to the client, the consumption is still OK.
I have renamed the title of this PR.
Thanks very much. It is an important review.
**[1]**:
https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L422
--
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]
