Skip to content

Initialize checkStamp in LatencyFaultToleranceImpl.FaultItem#10554

Open
vasiliy-mikhailov wants to merge 1 commit into
apache:developfrom
vasiliy-mikhailov:fix/faultitem-init-checkstamp
Open

Initialize checkStamp in LatencyFaultToleranceImpl.FaultItem#10554
vasiliy-mikhailov wants to merge 1 commit into
apache:developfrom
vasiliy-mikhailov:fix/faultitem-init-checkstamp

Conversation

@vasiliy-mikhailov

@vasiliy-mikhailov vasiliy-mikhailov commented Jun 27, 2026

Copy link
Copy Markdown

FaultItem.checkStamp is never initialized, so it defaults to 0. A freshly
created fault item is therefore immediately eligible for detection in
detectByOneRound() instead of waiting one detectInterval like every item
that has gone through updateFaultItem. This makes the very first detect round
fire a probe against a broker that was only just recorded.

Initialize checkStamp to now + detectInterval in the constructor, matching
the value updateFaultItem assigns. Added a regression test asserting the first
detectByOneRound() does not probe a freshly added item.

Verifying this change

The added LatencyFaultToleranceImplTest#testDetectByOneRoundRespectsDetectInterval fails on the current develop and passes with this change:

Before the fix (on develop):

$ mvn test -Dtest=LatencyFaultToleranceImplTest -pl client
Running org.apache.rocketmq.client.latency.LatencyFaultToleranceImplTest
Tests run: 5, Failures: 1, Errors: 0, Skipped: 1 <<< FAILURE!
	at org.apache.rocketmq.client.latency.LatencyFaultToleranceImplTest.testDetectByOneRoundRespectsDetectInterval(LatencyFaultToleranceImplTest.java:102)
[INFO] BUILD FAILURE

After the fix:

$ mvn test -Dtest=LatencyFaultToleranceImplTest -pl client
Running org.apache.rocketmq.client.latency.LatencyFaultToleranceImplTest
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

AI assistance disclosure

This contribution was produced with the help of an AI pipeline. The pipeline processed a large amount of source code to surface suspected bugs, reproduced a subset of them with failing unit tests and generated candidate fixes, and prepared pull requests from the ones that held up. Each PR was then reviewed and verified by a human before being opened: the fix and test were checked by hand and the test was confirmed to fail before the change and pass after.

FaultItem never set checkStamp, so it defaulted to 0 and detectByOneRound treated every fault item as ready to detect on every round, ignoring detectInterval. Initialize checkStamp to now plus detectInterval in the constructor.

@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

Initializes FaultItem.checkStamp in the constructor to now + detectInterval, preventing a freshly created item from being immediately eligible for detection in detectByOneRound().

Findings

  • [Info] LatencyFaultToleranceImpl.java:205 — The fix correctly mirrors the assignment in updateFaultItem(), ensuring consistent initial state. Good alignment.
  • [Info] LatencyFaultToleranceImplTest.java:89-103 — Regression test is well-structured: uses Mockito to verify detect() is never called for a freshly added item within the detect interval.

Verdict

Clean, minimal fix with a solid regression test. LGTM.


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

Summary

Initializes FaultItem.checkStamp to now + detectInterval in the constructor, preventing a freshly created item from being immediately eligible for detection in detectByOneRound().

Findings

  • [Info] LatencyFaultToleranceImpl.java:205 — The fix correctly mirrors the initialization done in updateFaultItem(). Clean one-line change.
  • [Info] LatencyFaultToleranceImplTest.java:92-103 — Good regression test. Using Mockito.never() to verify no detection occurs on a fresh item is the right approach. Setting detectInterval to 1 hour makes the test deterministic.

Suggestions

  • Consider whether LatencyFaultToleranceImpl.this.detectInterval could be zero at construction time (before setDetectInterval is called). If so, the initialization would still result in checkStamp = now, which is the same as the old behavior. The test sets it explicitly after construction, so this edge case may not be covered.

Verdict

Clean, well-targeted fix with a solid regression test. The concern above is minor and may not apply in practice since detectInterval is typically set before any FaultItem is created.


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