FrankChen021 commented on code in PR #19603:
URL: https://github.com/apache/druid/pull/19603#discussion_r3499360854
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java:
##########
@@ -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) {
Review Comment:
[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.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java:
##########
@@ -90,7 +90,7 @@ static class PeriodicDatabasePoll implements DatabasePoll
* leadership changes.
*/
final CompletableFuture<Void> firstPollCompletionFuture = new
CompletableFuture<>();
- long lastPollStartTimestampInMs = -1;
+ long lastPollStartTimestampInNanos = -1;
Review Comment:
[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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
