Skip to content

fix: Fix invalid time comparison #19603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vivek807 wants to merge 11 commits into
base: master
Choose a base branch
from
Open

fix: Fix invalid time comparison #19603

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e58b57c
Created deep-test.yml
vivek807 Sep 15, 2025
93f3450
Create deepbi-integration-test.yml
vivek807 Sep 18, 2025
de40832
Created deep-release.yml
vivek807 Sep 21, 2025
081d603
Merge branch 'apache:master' into master
nozjkoitop Nov 27, 2025
3d7f809
Replaced Release Workflow in master with a placeholder
nozjkoitop Nov 27, 2025
57df8d3
Licence added to a workflows
nozjkoitop Nov 27, 2025
128b3e9
renamed release flowfile for consistency
nozjkoitop Nov 27, 2025
99607d8
Merge branch 'apache:master' into master
nozjkoitop Feb 10, 2026
7029e8a
Merge branch 'apache:master' into master
nozjkoitop Feb 17, 2026
81c9773
Fix invalid clock comparison
vivek807 Jun 19, 2026
49b7535
Address review comment
vivek807 Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ static class PeriodicDatabasePoll implements DatabasePoll
* leadership changes.
*/
final CompletableFuture<Void> firstPollCompletionFuture = new CompletableFuture<>();
long lastPollStartTimestampInMs = -1;
long lastPollStartTimestampInNanos = -1;

@FrankChen021 FrankChen021 Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Do not use -1 as a nanoTime sentinel

This timestamp is now compared directly with System.nanoTime() readings, but System.nanoTime() has an arbitrary origin and is allowed to return negative values. If the manager has installed a new PeriodicDatabasePoll but its first task has not set this field yet, lastPollStartTimestampInNanos remains -1; on a JVM where the caller's checkStartTimeNanos is less than -1, forceOrWaitOngoingDatabasePoll will treat that unstarted poll as newer and return without polling or waiting. Use Long.MIN_VALUE, an explicit initialized flag, or a nullable/completion timestamp instead.

@kfaraz kfaraz Jul 2, 2026

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.

Yeah, I suppose null would be a better choice for this now.

}

/**
Expand Down Expand Up @@ -429,7 +429,7 @@ private Runnable createPollTaskForStartOrder(long startOrder, PeriodicDatabasePo
lock.lock();
try {
if (startOrder == currentStartPollingOrder) {
periodicDatabasePoll.lastPollStartTimestampInMs = System.currentTimeMillis();
periodicDatabasePoll.lastPollStartTimestampInNanos = System.nanoTime();
poll();
periodicDatabasePoll.firstPollCompletionFuture.complete(null);
latestDatabasePoll = periodicDatabasePoll;
Expand Down Expand Up @@ -556,22 +556,22 @@ boolean useLatestSnapshotIfWithinDelay()
@VisibleForTesting
void forceOrWaitOngoingDatabasePoll()
{
long checkStartTime = System.currentTimeMillis();
long checkStartTimeNanos = System.nanoTime();
ReentrantReadWriteLock.WriteLock lock = startStopPollLock.writeLock();
lock.lock();
try {
DatabasePoll latestDatabasePoll = this.latestDatabasePoll;
try {
//Verify if there was a periodic poll completed while we were waiting for the lock
if (latestDatabasePoll instanceof PeriodicDatabasePoll
&& ((PeriodicDatabasePoll) latestDatabasePoll).lastPollStartTimestampInMs > checkStartTime) {
&& ((PeriodicDatabasePoll) latestDatabasePoll).lastPollStartTimestampInNanos > checkStartTimeNanos) {
return;
}
// Verify if there was a on-demand poll completed while we were waiting for the lock
// Verify if there was an on-demand poll completed while we were waiting for the lock
if (latestDatabasePoll instanceof OnDemandDatabasePoll) {
long checkStartTimeNanos = TimeUnit.MILLISECONDS.toNanos(checkStartTime);
OnDemandDatabasePoll latestOnDemandPoll = (OnDemandDatabasePoll) latestDatabasePoll;
if (latestOnDemandPoll.initiationTimeNanos > checkStartTimeNanos) {

@FrankChen021 FrankChen021 Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Contended force calls still miss the poll they waited for

This still compares the existing on-demand poll's initiation time with the caller's check start. In the common contended path, thread A creates the OnDemandDatabasePoll and holds the write lock while polling; thread B enters forceOrWaitOngoingDatabasePoll after that initiation, waits for the same write lock until A's poll completes, then sees initiationTimeNanos <= checkStartTimeNanos and starts another database poll. That means the PR still serializes duplicate forced polls for callers that arrived during the actual poll, even though the previous poll completed after their call. Track and compare completion time, or otherwise record that the caller waited behind the in-progress on-demand poll, before returning/forcing a new poll.

Futures.getUnchecked(latestOnDemandPoll.pollCompletionFuture);
return;
}
}
Expand Down
Loading