bxfjb commented on PR #10655: URL: https://github.com/apache/rocketmq/pull/10655#issuecomment-5057939891
> ## Review by github-manager-bot > ### Summary > Adds gRPC-based benchmark producer and consumer tools (`GrpcProducer`, `GrpcConsumer`) using the `rocketmq-client-java` SDK, along with corresponding shell scripts for launching and shutting down the benchmarks. This extends the existing remoting-based benchmark suite with gRPC protocol support. > > ### Findings > * **[Warning]** `GrpcConsumer.java:103` — `successList` is a `LinkedList` shared across multiple scheduled tasks (lines 135–143) without synchronization. The scheduled consumer callback and the stats snapshot task both access it concurrently. Consider using `ConcurrentLinkedQueue` or `Collections.synchronizedList()` to avoid potential `ConcurrentModificationException` or lost counts. > * **[Warning]** `GrpcProducer.java:259` — `awaitTermination(1, TimeUnit.DAYS)` is effectively a no-timeout wait. If any producer thread fails to exit (e.g. stuck in a blocking send), the process will hang indefinitely. Consider a more reasonable 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. Note: the existing benchmark scripts (`produce.sh`, `consume.sh`) have the same pattern, so this is consistent but could be improved across the board. > * **[Info]** `GrpcProducer.java:147-148` — `accessKey`/`secretKey` default to `"ak"`/`"sk"`. This is fine for a benchmark tool, but worth a brief comment noting these are placeholder credentials for local testing only. > * **[Info]** `example/pom.xml` — The dependency uses `rocketmq-client-java-noshade` version `5.0.9-SNAPSHOT`. If this is intentional for development, consider adding a comment. If a release version is available, prefer that for reproducibility. > * **[Info]** `GrpcConsumer.java:152` — `System.currentTimeMillis()` is used for latency measurement. `System.nanoTime()` would be more accurate for measuring elapsed time (immune to wall-clock adjustments). Same applies to `GrpcProducer.java` timing. > > ### Verdict > Good addition to the benchmark toolkit — gRPC protocol coverage is a natural complement to the existing remoting benchmarks. The main concerns are the thread-safety issue in `GrpcConsumer` (should be fixed) and the infinite termination timeout in `GrpcProducer` (should be bounded). The rest are minor suggestions. > > _Automated review by github-manager-bot_ The Bot's comment is confusing: looks like some comments point to blank line number or referred a non-exist variable. -- 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]
