RockteMQ-AI commented on PR #10807: URL: https://github.com/apache/rocketmq/pull/10807#issuecomment-5205482855
## 🤖 Automated Review by RockteMQ-AI ## PR Review: apache/rocketmq#10807 ### 🔴 Critical None. ### 🟡 Warning - **`RemotingProtocolServer.java:391-393`** — Breaking the loop on any `Throwable` changes behavior: previously the loop silently continued, which could skip a single bad item and still process others. Now a single failure aborts the entire cleanup pass. This is acceptable if the method is invoked periodically, but verify that leaving remaining expired items until the next scheduled run is intentional. - **`RemotingProtocolServerTest.java:72`** — `Mockito.verify(executor, Mockito.atMost(2)).getQueue()` is a weak assertion; it permits 0, 1, or 2 invocations and does not strongly verify the break-on-failure behavior. Prefer `Mockito.times(2)` because the loop calls `getQueue()` once, fails, then `safeQueueSize()` calls it once more during logging. - **`RemotingProtocolServer.java:391`** — Ensure the `log` field exists in `RemotingProtocolServer` (not visible in the diff). Otherwise the change will not compile. ### 🟢 Suggestion - Add a test for the happy path to confirm normal expired-request cleanup still functions after the catch/block change. - Consider whether this failure should be logged at `error` rather than `warn`, since it represents an unexpected state in the cleanup path. --- <sub>Automated review. Please verify findings before acting on them.</sub> -- 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]
