RockteMQ-AI commented on PR #10655: URL: https://github.com/apache/rocketmq/pull/10655#issuecomment-5062018235
@bxfjb Thanks for pointing that out — you're right, the previous review had several factual inaccuracies. I re-checked the actual diff and here are the corrected findings: ### Corrected Findings - **[Warning]** `GrpcConsumer.java` — `snapshotList` (a `LinkedList<Long[]>`) is shared across the scheduled consumer callback and the stats snapshot task without synchronization. This could lead to `ConcurrentModificationException` or lost counts. Consider using `ConcurrentLinkedQueue` or `Collections.synchronizedList()`. - **[Warning]** `GrpcProducer.java` — `sendThreadPool.awaitTermination(Long.MAX_VALUE, TimeUnit.DAYS)` is effectively an infinite wait. If any producer thread fails to exit, the process will hang indefinitely. Consider a bounded timeout (e.g. 60 seconds) followed by `shutdownNow()` and a warning log. - **[Info]** `grpc-consumer.sh:18` / `grpc-producer.sh:18` — `$@` is unquoted. If any argument contains spaces, it will be word-split. Consider `"$@"` for robustness. (The existing benchmark scripts have the same pattern, so this is consistent but could be improved.) - **[Info]** `GrpcProducer.java` / `GrpcConsumer.java` — `accessKey`/`secretKey` default to `"12345678"`/`"rocketmq2"`. Fine for a benchmark tool, but worth a brief comment noting these are placeholder credentials for local testing only. - **[Info]** `GrpcConsumer.java` / `GrpcProducer.java` — `System.currentTimeMillis()` is used for latency measurement. `System.nanoTime()` would be more accurate for elapsed time (immune to wall-clock adjustments). Apologies for the earlier inaccuracies. The two **Warning** items (thread-safety of `snapshotList` and unbounded `awaitTermination`) are the most important ones to address. --- *Automated review by github-manager-bot (corrected)* -- 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]
