RongtongJin commented on code in PR #10514:
URL: https://github.com/apache/rocketmq/pull/10514#discussion_r3457613618
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/statictopic/TopicQueueMappingContext.java:
##########
@@ -20,6 +20,8 @@
import java.util.List;
public class TopicQueueMappingContext {
+ public static final TopicQueueMappingContext EMPTY = new
TopicQueueMappingContext(null, null, null, null, null);
Review Comment:
This singleton is risky as currently written. `TopicQueueMappingContext`
still exposes setters (`setTopic`, `setGlobalId`, `setMappingDetail`,
`setMappingItemList`, `setLeaderItem`, `setCurrentItem`), so `EMPTY` is not
actually immutable. Since this PR also does not appear to use the new constant,
I suggest removing it from this PR, or first making the sentinel/type truly
immutable before exposing a public shared instance.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/RemotingCommand.java:
##########
@@ -635,12 +658,20 @@ public void setSerializeTypeCurrentRPC(SerializeType
serializeTypeCurrentRPC) {
this.serializeTypeCurrentRPC = serializeTypeCurrentRPC;
}
- public Stopwatch getProcessTimer() {
- return processTimer;
+ public long processTimerElapsedMs() {
+ if (processTimerNanos == 0) {
+ return 0;
+ }
+ return (System.nanoTime() - processTimerNanos) / 1_000_000;
+ }
+
+ @Deprecated
+ public com.google.common.base.Stopwatch getProcessTimer() {
Review Comment:
Keeping a deprecated getter helps source compatibility, but this
implementation no longer represents the original request timer: it returns a
newly started `Stopwatch`, so any downstream code that still calls
`getProcessTimer().elapsed(...)` will silently observe near-zero latency. This
PR also removes the old public `setProcessTimer(Stopwatch)` method, which can
break downstream source/binary compatibility. Please either preserve deprecated
adapter methods backed by `processTimerNanos`, or make the API break explicit
instead of returning a misleading timer.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/netty/NettyRemotingServer.java:
##########
@@ -581,13 +581,17 @@ public void
channelWritabilityChanged(ChannelHandlerContext ctx) throws Exceptio
if (channel.isWritable()) {
if (!channel.config().isAutoRead()) {
channel.config().setAutoRead(true);
- log.info("Channel[{}] turns writable, bytes to buffer
before changing channel to un-writable: {}",
- RemotingHelper.parseChannelRemoteAddr(channel),
channel.bytesBeforeUnwritable());
+ if (log.isDebugEnabled()) {
+ log.debug("Channel[{}] turns writable, bytes to buffer
before changing channel to un-writable: {}",
+ RemotingHelper.parseChannelRemoteAddr(channel),
channel.bytesBeforeUnwritable());
+ }
}
} else {
channel.config().setAutoRead(false);
- log.warn("Channel[{}] auto-read is disabled, bytes to drain
before it turns writable: {}",
- RemotingHelper.parseChannelRemoteAddr(channel),
channel.bytesBeforeWritable());
+ if (log.isDebugEnabled()) {
+ log.debug("Channel[{}] auto-read is disabled, bytes to
drain before it turns writable: {}",
Review Comment:
I suggest keeping the original log level for this backpressure signal.
Disabling auto-read is operationally important when diagnosing channel pressure
or slow consumers, and downgrading it from `warn` to `debug` can hide that
signal in production. If the current log volume is too high, a rate-limited
warn or a configuration switch would keep observability while still reducing
noise.
--
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]
