RockteMQ-AI commented on PR #342:
URL: https://github.com/apache/rocketmq-clients/pull/342#issuecomment-4691800282
## Review by github-manager-bot
### Summary
Adds PullConsumer API interfaces for RocketMQ 5.0, providing manual message
queue assignment and pull-based consumption similar to LitePullConsumer from
4.0, along with supporting builder, listener, and MessageQueue interfaces.
### Findings
- **[Critical]** PullConsumer.java:44-128 — Multiple Javadoc `@param` tags
are malformed, using file paths like `@repos/apache_rocketmq-clients/cpp/...`
instead of parameter names. This will break Javadoc generation and make the API
documentation unusable.
- **[Critical]** PullConsumerBuilder.java:34-75 — All `@param` tags contain
malformed file path references instead of parameter names, preventing proper
Javadoc generation.
- **[Critical]** TopicMessageQueueChangeListener.java:28 — Malformed
`@param` tags with file path references instead of parameter names.
- **[Warning]** PullConsumer.java:68,76,119,128 — Javadoc `{@link}` syntax
is broken, referencing external files instead of using standard Java link
syntax like `{@link #poll(Duration)}`.
- **[Warning]** PullConsumer.java:103 — Javadoc references
`Optional#empty()` with broken syntax; should be `{@link Optional#empty()}`.
- **[Warning]** PullConsumer.java:96 — `offsetForTimestamp(MessageQueue
messageQueue, Long timestamp)` uses boxed `Long` instead of primitive `long`.
If null is not a valid input, use primitive for clarity and performance.
- **[Info]** PullConsumer.java:56,84 — `assign()`, `pause()`, and `resume()`
methods don't document behavior for null or empty collections, or for queues
not currently assigned.
- **[Info]** PullConsumer.java — No thread-safety documentation. Specify
whether methods can be called concurrently from multiple threads.
- **[Info]** PullConsumer.java:68 — `poll(Duration timeout)` doesn't declare
checked exceptions. Document what happens on network errors, consumer closure,
or interruption (does it throw, return empty list, etc?).
- **[Info]** MessageQueue.java — Interface will be used in Collections (see
`Set<MessageQueue>` in listener). Document `equals()` and `hashCode()` contract
expectations.
- **[Info]** PullConsumer.java — No method to query current assignment or
check if a specific queue is paused. Consider if these are needed for
debugging/monitoring.
### Suggestions
1. **Fix all Javadoc tags immediately** — Replace all malformed `@repos/...`
and `@gh_2.65.0_linux_amd64/...` references with proper parameter names and
`{@link}` syntax. This is a blocker for the PR.
2. **Add null-safety documentation** — Specify behavior when null or empty
collections are passed to `assign()`, `pause()`, `resume()`.
3. **Document thread safety** — Add class-level Javadoc stating whether the
interface is thread-safe and which methods can be called concurrently.
4. **Clarify exception behavior** — Document what exceptions `poll()` might
throw or return on error conditions.
5. **Consider adding query methods** — Methods like `assignment()` to get
current assignments or `isPaused(MessageQueue)` could be valuable for debugging
and monitoring, though not critical for initial implementation.
6. **Use primitive types where appropriate** — Change `Long timestamp` to
`long timestamp` in `offsetForTimestamp()` unless null is a valid input that
needs special handling.
---
*Automated review by github-manager-bot*
--
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]
