wy471x commented on PR #6902:
URL: https://github.com/apache/shenyu/pull/6902#issuecomment-5303103306
> ## Review: #6902 — feat: implement MQTT Last Will and Testament (LWT)
> **Verdict: ✅ Approve (with one consistency follow-up)**
>
> Solid, well-tested feature implementation. The lifecycle handling is
correct and the test coverage is excellent.
>
> ### What's correct
> * **The `MqttFactory` DISCONNECT fix is the linchpin.** Original code had
`case PUBACK: case DISCONNECT: default: break;` — so DISCONNECT messages were
silently dropped and `Disconnect.disconnect()` was _never invoked_. Without
this fix, graceful-disconnect will removal couldn't work at all. Good catch,
and it's required for the rest of the feature to function.
> * **Correct will lifecycle:**
>
> * CONNECT with `isWillFlag()` → `WillRepository.add(channel, will)` ✅
> * Graceful DISCONNECT → `WillRepository.remove(channel)` → will never
fires ✅
> * Ungraceful disconnect → `MqttTransportHandler.channelInactive()` sees
the will present → `Publish.publishWill(will)` → removes it ✅
> * The inactive channel is excluded from receiving its own will
(`channel.isActive()` guard) ✅
> * No double-publish: `channelInactive` removes the will immediately and
Netty fires it once per close ✅
> * **`WillRepository`** is a clean `ConcurrentHashMap<Channel, WillEntry>`
keyed by Channel (not clientId), so reconnects with a new channel don't
collide, and `testReplaceWillEntryOnReconnect` covers the replace path.
> * **`publishWill`** null-guards topic/message, derives a valid packetId (0
for AT_MOST_ONCE, random otherwise), and respects the will QoS/retain flags.
> * **Test coverage is genuinely thorough:** `ConnectTest` (store / no-will
/ qos0 / retain), `DisconnectTest` (clears will / no will / removes channel),
`MqttTransportHandlerTest` (fires+removes / no will / post-disconnect),
`PublishWillTest` (active / inactive-skip / empty / qos+retain), and
`WillRepositoryTest`. The pom changes (junit-jupiter, mockito, `--add-opens`
for JDK 17) are the right scaffolding to support them.
>
> ### Suggestion (non-blocking)
> 1. **Wildcard-aware will delivery.** `Publish.publishWill` uses
`SubscribeRepository.get(will.getTopic())` — an _exact_ lookup. A client
subscribed to e.g. `status/#` will **not** receive a will published to
`status/client-001`, even though normal publish routing should match it. Since
#6906 (wildcard matching) adds `getChannelsByTopic`, it would be consistent to
route the will through that here too. Not blocking (LWT-to-wildcard-subscriber
is an edge case), but worth aligning.
> 2. **Retained will semantics.** A will with `retain=true` is sent with the
RETAIN flag, but there's no evidence the broker persists retained messages for
later subscribers. That's a broader retained-message gap, outside this PR's
scope — just flagging so it's tracked.
>
> ### Verdict
> Approving. The implementation is correct, the essential DISCONNECT routing
bug is fixed, and the tests back the behavior end-to-end. Address the
wildcard-delivery point as a small follow-up.
Thank you for the code review on this PR.
Fix: Wildcard-aware will delivery — previously Publish.publishWill used
SubscribeRepository.get(topic), an exact-match lookup, so a client subscribed
to status/# never received a will published to status/client-001.
- Ported TopicMatcher and SubscribeRepository.getChannelsByTopic
(identical to PR #6906 code) into the LWT branch
- publishWill now routes through getChannelsByTopic, consistent with
normal publish routing
- Updated PublishWillTest/MqttTransportHandlerTest stubs; added
wildcard-subscriber test and ported TopicMatcherTest/SubscribeRepositoryTest
- Fixed checkstyle violation (== null → Objects.isNull)
--
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]
