Skip to content

add comments to pop/ack/changInvisibleTime, No Code change#10402

Open
winglechen wants to merge 211 commits into
apache:developfrom
wolforest:comment
Open

add comments to pop/ack/changInvisibleTime, No Code change#10402
winglechen wants to merge 211 commits into
apache:developfrom
wolforest:comment

Conversation

@winglechen

Copy link
Copy Markdown

约1200行注释,无代码修改

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

This PR adds approximately 1200 lines of Javadoc and inline comments across 30+ files in the Pop consumer, ACK, ChangeInvisibleTime, and related proxy/store code paths. No functional code changes.

Findings

  • [Positive] PopLongPollingService.java — The class-level Javadoc clearly explains the core responsibilities (suspend/wake/timeout/retry-bridge/cleanup). This is very helpful for onboarding.

  • [Positive] PopConsumerCache.java — Good documentation of the cache data structure (active vs removed trees) and the background scan lifecycle.

  • [Warning] Multiple files — Several comments describe intended behavior rather than actual behavior. For example, inline comments like // offset is ok or // init context params and validate describe what the code should do but do not explain non-obvious logic. Comments should explain why, not just restate what the code does.

  • [Warning] DefaultMessageStore.java — Comments added inside getMessage() method (e.g., // offset too small, // offset overflow one, // get message, roll to next file if needed) are essentially paraphrasing the existing status enum names. These add visual noise without improving comprehension.

  • [Info] With 30+ files touched, this PR is large for a "comments only" change. Reviewers may have difficulty verifying comment accuracy at this scale. Consider splitting into smaller PRs by module (e.g., broker/pop first, then proxy, then store) to make reviews more focused.

Suggestions

  1. Focus comments on why (design decisions, non-obvious constraints, edge cases) rather than what (restating the code).
  2. Remove comments that simply paraphrase variable/method names (e.g., // init context vars before parameter initialization).
  3. Consider breaking into 2-3 smaller PRs by module for easier review.

Automated review by github-manager-bot

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot (Incremental)

Summary

5 new commits since the previous review, adding Javadoc and inline comments to SendMessageProcessor, HookUtils, TimerMessageStore, and BrokerConfig. All changes are documentation-only — no functional code modifications.

Findings

  • [Positive] HookUtils.java — The class-level Javadoc clearly documents the 4-step processing pipeline (checkBeforePutMessagecheckInnerBatchhandleScheduleMessagehandleLmqQuota) and the abort-on-non-null contract. This is very helpful for understanding the send-message pre-processing flow.

  • [Positive] HookUtils.handleScheduleMessage — The inline comments clarifying the timer vs delay-level routing logic are accurate. The note "normal message or committed message can be delayed" correctly describes the tranType guard.

  • [Positive] HookUtils.transformTimerMessage — The "calculate deliver time" comment and the expanded explanation of the isRolledTimerMessage double-check path improve readability of this non-trivial method.

  • [Positive] TimerMessageStore.isReject — The 3-tier flow control Javadoc (≤1x admit, 1x-2x probabilistic, ≥2x reject) is clear and matches the implementation. The note "always return false with default config" is a useful operational hint.

  • [Info] SendMessageProcessor.java:117 — The comment // no batch message after 5.0 on the isBatch() branch is helpful context. Consider also noting whether this branch can be safely deprecated/removed in a future major version cleanup.

  • [Info] HookUtils.java:167 — Changed "Delay Delivery" to "Delay Delivery, useless with default config". This is accurate when timerWheelEnable=true (default), but when the timer wheel is disabled, delay-level delivery is still the primary scheduling mechanism. Consider rephrasing to "Delay Delivery (bypassed when timer wheel is enabled)" for precision.

Verdict

All new comments are accurate and improve code readability. No issues found. LGTM.


Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants