fix: Fix invalid time comparison#19603
Conversation
534f417 to
81c9773
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 1 of 1 changed files.
This is an automated review by Codex GPT-5.5
Add Unit test for same
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 0 |
| P3 | 1 |
| Total | 1 |
Reviewed 2 of 2 changed files.
This is an automated review by Codex GPT-5.5
| try { | ||
| caller.start(); | ||
| // Allow the caller to capture checkStartTimeNanos and block on the write lock. | ||
| Thread.sleep(100); |
There was a problem hiding this comment.
[P3] Replace timing sleep with deterministic thread coordination
The new regression test depends on the caller thread being scheduled within this fixed 100 ms window so it captures checkStartTimeNanos before concurrentPoll is created. On a slow or overloaded CI worker, the caller can start after the sleep, making concurrentPoll.initiationTimeNanos <= checkStartTimeNanos; then the production branch under test is skipped and the caller may run its own poll or return early, causing a flaky failure or a false pass. Please coordinate on the lock queue/state, e.g. wait until the caller is queued on the write lock before creating concurrentPoll, instead of relying on sleep timing.
|
@vivek807 |
Fixes #19602.
Description
forceOrWaitOngoingDatabasePollinSqlSegmentsMetadataManagerchecks whether a concurrentOnDemandDatabasePollthat completed while the calling thread waited for the write lock makes a new poll unnecessary. This check was always false due to a clock mismatch, causing an extra database poll on every contended call.Fixed invalid clock comparison in
forceOrWaitOngoingDatabasePollOnDemandDatabasePoll.initiationTimeNanosis assigned viaSystem.nanoTime(), which is a monotonic clock with an arbitrary JVM-internal reference point.The comparison value was constructed as:
System.currentTimeMillis() returns milliseconds since the Unix epoch (~1.7 × 10¹² ms). Converting to nanoseconds yields ~1.7 × 10²¹ ns, which is orders of magnitude larger than any System.nanoTime() value. The condition
initiationTimeNanos > checkStartTimeNanoswas therefore always false, so an in-progress on-demand poll was never recognised as sufficient.This PR has: