Aias00 commented on PR #10585:
URL: https://github.com/apache/rocketmq/pull/10585#issuecomment-4951539301
Follow-up to the review suggestion about auditing other
`getClientSettings()` call sites — and a note on the per-call allocation.
## Call-site audit
`getClientSettings(ProxyContext ctx)` returns `null` when the client has not
yet registered settings (`CLIENT_SETTINGS_MAP` has no entry for
`ctx.getClientID()`). There are three call sites in the proxy:
| Call site | Null handling |
| --- | --- |
| `ReceiveMessageActivity#receiveMessage` (line 67) | Fixed by this PR —
falls back to `getDefaultConsumerSettings()`. |
| `ClientActivity#heartbeat` (line 102) | Already null-guarded — replies
`UNRECOGNIZED_CLIENT_TYPE` ("cannot find client settings for this client") and
returns. |
| `ClientActivity#processClientSettings` (line 406) | Called immediately
after `updateClientSettings(...)` on line 405 puts the entry, so it is non-null
on the same thread. (A 5 s background cleaner can evict stale consumer entries
via `onWaitEnd`, but it only removes entries whose channel is gone — not an
active client mid-telemetry.) |
Producer activities (`SendMessageActivity`, `RecallMessageActivity`,
`ForwardMessageToDLQActivity`) do not read client settings at all, and
`GrpcClientChannel#getChannelExtendAttribute` null-guards and returns `null`.
So `ReceiveMessageActivity` was the only unguarded site; no further changes
are needed.
## On the per-call `getDefaultConsumerSettings()` allocation (the `[Info]`
note)
Intentionally not cached. This path is hit only before a client's first
settings telemetry — i.e. once per client at most, never on the hot receive
loop (the non-null `getClientSettings` path is hot). Caching the default would
pin a `Settings` built from the `SubscriptionGroupConfig` at first use and
serve stale `maxAttempts`/backoff values if that config changed, trading a
one-time cold-path allocation for a correctness risk on a code path that is
already rarely taken. Keeping it allocation-per-call keeps the default fresh at
negligible cost.
`mvn -pl proxy
-Dtest=ReceiveMessageActivityTest,PopMessageResultFilterImplTest,GrpcClientSettingsManagerTest
test` → 21 passed, 0 failures.
--
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]
