Skip to content

OAK-12037 : remove usage of Guava's Monitor from oak-lucene #2660

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

Merged
rishabhdaim merged 1 commit into from
Jan 15, 2026
+21 −13
Merged

OAK-12037 : remove usage of Guava's Monitor from oak-lucene #2660

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 21 additions & 13 deletions oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexCopier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import javax.management.openmbean.CompositeDataSupport;
import javax.management.openmbean.CompositeType;
Expand All @@ -40,7 +43,6 @@
import javax.management.openmbean.TabularDataSupport;
import javax.management.openmbean.TabularType;

import org.apache.jackrabbit.guava.common.util.concurrent.Monitor;
import org.apache.commons.io.FileUtils;
import org.apache.jackrabbit.oak.commons.collections.IterableUtils;
import org.apache.jackrabbit.oak.commons.collections.SetUtils;
Expand Down Expand Up @@ -95,7 +97,8 @@
private final AtomicLong downloadTime = new AtomicLong();
private final AtomicLong uploadTime = new AtomicLong();

private final Monitor copyCompletionMonitor = new Monitor();
private final Lock copyCompletionLock = new ReentrantLock();
private final Condition notCopyingCondition = copyCompletionLock.newCondition();

private final Map<String, String> indexPathVersionMapping = new ConcurrentHashMap<>();
private final ConcurrentMap<String, LocalIndexFile> failedToDeleteFiles = new ConcurrentHashMap<>();
Expand Down Expand Up @@ -363,28 +366,31 @@
* @param timeoutMillis
*/
public void waitForCopyCompletion(LocalIndexFile file, long timeoutMillis) {
final Monitor.Guard notCopyingGuard = new Monitor.Guard(copyCompletionMonitor) {
@Override
public boolean isSatisfied() {
return !isCopyInProgress(file);
}
};
long localLength = file.actualSize();
long lastLocalLength = localLength;

boolean notCopying = !isCopyInProgress(file);
while (!notCopying) {
final long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeoutMillis);
copyCompletionLock.lock();
Copy link
Contributor

@mbaedke mbaedke Jan 6, 2026

Choose a reason for hiding this comment

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

How is that working with the timeout?

Copy link
Contributor Author

@rishabhdaim rishabhdaim Jan 7, 2026

Choose a reason for hiding this comment

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

by following this logic, we ensure that we block for only an absolute time in future.

On each inner-loop iteration you recompute remaining = deadline - System.nanoTime()

This is to ensure that regardless of how many times the threads are woken, we only wait for timeoutMillis.

Copy link
Contributor

@mbaedke mbaedke Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not asking what the timeout is there for, I'm asking how copyCompletionLock.lock() is supposed to work here. It might break the whole timeout idea. Shouldn't we use copyCompletionLock.tryLock(long, TimeUnit) there?

Copy link
Contributor Author

@rishabhdaim rishabhdaim Jan 15, 2026

Choose a reason for hiding this comment

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

copyCompletionLock.lock() is required to wait for the condition i.e. isCopyInProgress(file) in this case. I am not sure why do we need copyCompletionLock.tryLock(long, TimeUnit) here.

Also, nanos is used to for better time management and it was suggested by cursor and other AI tools over timeout in millis.

Copy link
Contributor

@mbaedke mbaedke Jan 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. I though that copyCompletionLock.lock() might lock indefinitely, but with the logic implemented here that's actually not possible, afaics.

try {
if (log.isDebugEnabled()) {
log.debug("Checking for copy completion of {} - {}", file.getKey(), file.copyLog());
}
notCopying = copyCompletionMonitor.enterWhen(notCopyingGuard, timeoutMillis, TimeUnit.MILLISECONDS);
if (notCopying) {
copyCompletionMonitor.leave();
while (isCopyInProgress(file)) {
long remaining = deadline - System.nanoTime();
if (remaining <= 0) {
// timeout
break;
}
notCopyingCondition.awaitNanos(remaining);

Check warning on line 386 in oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/IndexCopier.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Do something with the "long" value returned by "awaitNanos". 


See more on https://sonarcloud.io/project/issues?id=org.apache.jackrabbit%3Ajackrabbit-oak&issues=AZseN8U6ekvuHONwFd01&open=AZseN8U6ekvuHONwFd01&pullRequest=2660
Copy link
Contributor

@mbaedke mbaedke Jan 6, 2026

Choose a reason for hiding this comment

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

This whole nano calculation seems clumsy. Why not just notCopyingCondition.await(timeoutMillis, TimeUnit.MILLISECONDS) ?

Copy link
Member

@thomasmueller thomasmueller Jan 6, 2026

Choose a reason for hiding this comment

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

AFAIK the thread could be woken up in some cases, and so a loop is needed, and so a calculation is needed.

Copy link
Contributor

@mbaedke mbaedke Jan 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, a loop is needed . But just use notCopyingCondition.await(timeoutMillis, TimeUnit.MILLISECONDS). There is no reason to convert to nanos, is there?

}
notCopying = !isCopyInProgress(file);
} catch (InterruptedException e) {
// ignore and reset interrupt flag
Thread.currentThread().interrupt();
} finally {
copyCompletionLock.unlock();
}

localLength = file.actualSize();
Expand All @@ -405,11 +411,13 @@
}

public void doneCopy(LocalIndexFile file, long start) {
copyCompletionMonitor.enter();
copyCompletionLock.lock();
try {
copyInProgressFiles.remove(file);
// wake up any threads waiting in waitForCopyCompletion(...)
notCopyingCondition.signalAll();
} finally {
copyCompletionMonitor.leave();
copyCompletionLock.unlock();
}
copyInProgressCount.decrementAndGet();
copyInProgressSize.addAndGet(-file.getSize());
Expand Down
Loading