Repository navigation
test: fix heartbeat failure isolation test after publisher startup refactor - #7403
Conversation
ShenyuClientRegisterEventPublisher#start now schedules the URI heartbeat inside ShenyuClientURIExecutorSubscriber#start instead of the constructor, so the executor queue stays empty until start() is called. The test read the queue directly and failed with NoSuchElementException; start the subscriber before draining the scheduled task.
|
Heads-up: this overlaps with my #7402 for the same HeartbeatFailureIsolationTest failure. Both patches add subscriber.start() before draining the scheduled task. I rechecked the scheduler: start() uses a 30-second initial delay, so #7403 does not schedule an immediate heartbeat before its fixture setup under normal test execution. #7402 does initialize the URI list before scheduling, which is marginally more robust if the test is stalled for a long time, but the intended fix is otherwise equivalent. To avoid duplicate maintenance, please merge only one; I am happy to close #7402 if maintainers prefer this PR. |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Correct one-line fix. ShenyuClientURIExecutorSubscriber no longer schedules the heartbeat task in its constructor — start() does, guarded by started.compareAndSet(false, true) — so until start() is called the queue is empty and executor.getQueue().iterator().next() at line 57 throws NoSuchElementException. Adding subscriber.start() restores the pending task the test then drives by hand.
No leak introduced: start() is idempotent and the test's finally still calls executor.shutdownNow().
Nit: this is the same one-line fix as #7402. Please coordinate with @sunnysabor and close one of the two. #7402 places the start() call after uris is populated, which reads slightly more naturally (start heartbeating once there is something to heartbeat), but either placement works here — the task carries a 30-second initial delay and is only ever executed manually in this test.
This repairs
HeartbeatFailureIsolationTest, the regression test added in #7260 and left broken by the #7082 register publisher startup refactor.Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.Summary
ShenyuClientURIExecutorSubscriberused to schedule its periodic heartbeat from the constructor. #7082 moved thescheduleAtFixedRatecall into a new idempotentstart()method, so the executor queue stays empty untilstart()is called.HeartbeatFailureIsolationTestreflected the executor queue right after construction, hitjava.util.NoSuchElementExceptionat line 57, and failed theshenyu-client-coremodule tests.Changes:
HeartbeatFailureIsolationTest.failureDoesNotSuppressOtherUrisOrSubsequentTicks(HeartbeatFailureIsolationTest.java:48) — callssubscriber.start()before draining the scheduled task from the queue, matching the publisher lifecycle introduced by fix(client): make register publisher startup idempotent #7082. No production code changed.Test Cases:
HeartbeatFailureIsolationTest.failureDoesNotSuppressOtherUrisOrSubsequentTicks— runs two heartbeat ticks with a repository that throws for one of two URIs, then verifies the failing heartbeat is retried, the healthy URI is still reported, and the periodic task remains schedulable (assertFalse(task.isDone())).Verification
JAVA_HOME=<JDK 21> ./mvnw -pl shenyu-client/shenyu-client-core -am clean install -Dmaven.javadoc.skip=true— 101 tests in shenyu-client-core, 0 failures, 0 errors; checkstyle and RAT (bound tovalidate) pass.java.util.NoSuchElementExceptionatHeartbeatFailureIsolationTest.java:57; with it the test passes.Related to #7260.