Copilot commented on code in PR #9273:
URL: https://github.com/apache/ozone/pull/9273#discussion_r2512213620
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java:
##########
@@ -92,6 +99,36 @@ boolean isPreviousPurgeTransactionFlushed() throws
IOException {
return true;
}
+ /**
+ * A specialized implementation of {@link BackgroundTaskQueue} that modifies
+ * the behavior of added tasks to utilize a read lock during execution.
+ *
+ * This class ensures that every {@link BackgroundTask} added to the queue
+ * is wrapped such that its execution acquires a read lock via
+ * {@code getBootstrapStateLock().acquireReadLock()} before performing any
+ * operations. The lock is automatically released upon task completion or
+ * exception, ensuring safe concurrent execution of tasks within the service
when running along with bootstrap flow.
Review Comment:
[nitpick] The Javadoc comment should end with "ensures safe concurrent
execution of tasks within the service when running alongside bootstrap flow."
(changed "along with" to "alongside" for better grammar).
```suggestion
* exception, ensuring safe concurrent execution of tasks within the
service when running alongside bootstrap flow.
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/BootstrapStateHandler.java:
##########
@@ -17,28 +17,31 @@
package org.apache.hadoop.ozone.lock;
-import java.util.concurrent.Semaphore;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ratis.util.UncheckedAutoCloseable;
/** Bootstrap state handler interface. */
public interface BootstrapStateHandler {
Lock getBootstrapStateLock();
- /** Bootstrap state handler lock implementation. */
- class Lock implements AutoCloseable {
- private final Semaphore semaphore = new Semaphore(1);
+ /** Bootstrap state handler lock implementation. Should be always acquired
before opening any snapshot to avoid
+ * deadlocks*/
+ class Lock {
+ private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
- public Lock lock() throws InterruptedException {
- semaphore.acquire();
- return this;
+ private UncheckedAutoCloseable lock(boolean readLock) {
+ java.util.concurrent.locks.Lock lock = readLock ?
readWriteLock.readLock() : readWriteLock.writeLock();
+ lock.lock();
+ return lock::unlock;
}
- public void unlock() {
- semaphore.release();
+ public UncheckedAutoCloseable acquireWriteLock() throws
InterruptedException {
+ return lock(false);
}
- @Override
- public void close() {
- unlock();
+ public UncheckedAutoCloseable acquireReadLock() throws
InterruptedException {
+ return lock(true);
Review Comment:
The methods `acquireWriteLock()` and `acquireReadLock()` declare `throws
InterruptedException`, but they cannot actually throw this exception. The
underlying `lock.lock()` call (line 35) is a blocking operation that doesn't
throw `InterruptedException`.
If you need interruptible locking, use `lock.lockInterruptibly()` instead.
Otherwise, remove the `throws InterruptedException` declaration from both
methods.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/DBCheckpointServlet.java:
##########
@@ -394,17 +395,21 @@ public BootstrapStateHandler.Lock getBootstrapStateLock()
{
* This lock is a no-op but can overridden by child classes.
*/
public static class Lock extends BootstrapStateHandler.Lock {
+
+ private final UncheckedAutoCloseable noopLock = () -> {
+ };
+
public Lock() {
Review Comment:
Lock has the same name as its supertype
[org.apache.hadoop.ozone.lock.BootstrapStateHandler$Lock](1).
```suggestion
public static class NoOpLock extends BootstrapStateHandler.Lock {
private final UncheckedAutoCloseable noopLock = () -> {
};
public NoOpLock() {
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/lock/BootstrapStateHandler.java:
##########
@@ -17,28 +17,31 @@
package org.apache.hadoop.ozone.lock;
-import java.util.concurrent.Semaphore;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ratis.util.UncheckedAutoCloseable;
/** Bootstrap state handler interface. */
public interface BootstrapStateHandler {
Lock getBootstrapStateLock();
- /** Bootstrap state handler lock implementation. */
- class Lock implements AutoCloseable {
- private final Semaphore semaphore = new Semaphore(1);
+ /** Bootstrap state handler lock implementation. Should be always acquired
before opening any snapshot to avoid
+ * deadlocks*/
Review Comment:
Missing space after period in the comment. Should be "Should be always
acquired before opening any snapshot to avoid deadlocks."
```suggestion
* deadlocks */
```
--
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]
