jelmini commented on code in PR #1204:
URL: https://github.com/apache/jackrabbit-oak/pull/1204#discussion_r1394377208
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java:
##########
@@ -34,8 +37,16 @@ public class AzureRepositoryLock implements RepositoryLock {
private static final Logger log =
LoggerFactory.getLogger(AzureRepositoryLock.class);
private static final int TIMEOUT_SEC =
Integer.getInteger("oak.segment.azure.lock.timeout", 0);
+ private static final Integer LEASE_RENEWAL_TIMEOUT_MS = 5000;
- private static int INTERVAL = 60;
+ public static final String INTERVAL_PROP =
"oak.segment.azure.lock.leaseDuration";
+ private static int INTERVAL = Integer.getInteger(INTERVAL_PROP, 60);
+
+ public static final String RENEWAL_FREQUENCY_PROP =
"oak.segment.azure.lock.leaseRenewalFrequency";
+ private static int RENEWAL_FREQUENCY =
Integer.getInteger(RENEWAL_FREQUENCY_PROP, 5);
Review Comment:
nitpick: the name `frequency` might be misleading, as a frequency is defined
as number of events over a unit of time, like 5 renewals per second. But here
we actually mean renewals every 5 seconds. Maybe `RENEWAL_INTERVAL`?
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java:
##########
@@ -45,19 +56,27 @@ public class AzureRepositoryLock implements RepositoryLock {
private final int timeoutSec;
+ private WriteAccessController writeAccessController;
+
private String leaseId;
private volatile boolean doUpdate;
- public AzureRepositoryLock(CloudBlockBlob blob, Runnable shutdownHook) {
- this(blob, shutdownHook, TIMEOUT_SEC);
+ public AzureRepositoryLock(CloudBlockBlob blob, Runnable shutdownHook,
WriteAccessController writeAccessController) {
+ this(blob, shutdownHook, writeAccessController, TIMEOUT_SEC);
}
- public AzureRepositoryLock(CloudBlockBlob blob, Runnable shutdownHook, int
timeoutSec) {
+ public AzureRepositoryLock(CloudBlockBlob blob, Runnable shutdownHook,
WriteAccessController writeAccessController, int timeoutSec) {
this.shutdownHook = shutdownHook;
this.blob = blob;
this.executor = Executors.newSingleThreadExecutor();
this.timeoutSec = timeoutSec;
+ this.writeAccessController = writeAccessController;
+
+ if (INTERVAL < RENEWAL_FREQUENCY || INTERVAL <
TIME_TO_WAIT_BEFORE_WRITE_BLOCK) {
Review Comment:
I would also check that `RENEWAL_FREQUENCY` is lower than
`TIME_TO_WAIT_BEFORE_WRITE_BLOCK` (which actually makes the check `INTERVAL <
RENEWAL_FREQUENCY` redundant)
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java:
##########
@@ -34,8 +37,16 @@ public class AzureRepositoryLock implements RepositoryLock {
private static final Logger log =
LoggerFactory.getLogger(AzureRepositoryLock.class);
private static final int TIMEOUT_SEC =
Integer.getInteger("oak.segment.azure.lock.timeout", 0);
+ private static final Integer LEASE_RENEWAL_TIMEOUT_MS = 5000;
- private static int INTERVAL = 60;
+ public static final String INTERVAL_PROP =
"oak.segment.azure.lock.leaseDuration";
Review Comment:
For all the new system properties, I would indicate in the name if it's
seconds or millis, otherwise it can be confusing. Something like
`oak.segment.azure.lock.leaseDurationInSec`
##########
oak-segment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureArchiveManagerTest.java:
##########
@@ -465,6 +475,80 @@ public void
testCollectBlobReferencesDoesNotFailWhenFileIsMissing() throws URISy
}
}
+ @Test
+ public void testWriteAfterLoosingRepoLock() throws URISyntaxException,
InvalidFileStoreVersionException, IOException, CommitFailedException,
StorageException, InterruptedException {
+ CloudBlobDirectory oakDirectory =
container.getDirectoryReference("oak");
+ AzurePersistence rwPersistence = new AzurePersistence(oakDirectory);
+
+ CloudBlockBlob blob = container.getBlockBlobReference("oak/repo.lock");
+
+ CloudBlockBlob blobMocked = Mockito.spy(blob);
+
+ Mockito
+ .doCallRealMethod()
+ .when(blobMocked).renewLease(Mockito.any(), Mockito.any(),
Mockito.any());
+
+ AzurePersistence mockedRwPersistence = Mockito.spy(rwPersistence);
+ WriteAccessController writeAccessController = new
WriteAccessController();
+ AzureRepositoryLock azureRepositoryLock = new
AzureRepositoryLock(blobMocked, () -> {}, writeAccessController);
+ AzureArchiveManager azureArchiveManager = new
AzureArchiveManager(oakDirectory, new IOMonitorAdapter(), new
FileStoreMonitorAdapter(), writeAccessController);
+
+
+ Mockito
+ .doAnswer(invocation -> azureRepositoryLock.lock())
+ .when(mockedRwPersistence).lockRepository();
+
+ Mockito
+ .doReturn(azureArchiveManager)
+
.when(mockedRwPersistence).createArchiveManager(Mockito.anyBoolean(),
Mockito.anyBoolean(), Mockito.any(), Mockito.any(), Mockito.any());
+ Mockito
+ .doReturn(new AzureJournalFile(oakDirectory, "journal.log",
writeAccessController))
+ .when(mockedRwPersistence).getJournalFile();
+
+ FileStore rwFileStore =
FileStoreBuilder.fileStoreBuilder(folder.newFolder()).withCustomPersistence(mockedRwPersistence).build();
+ SegmentNodeStore segmentNodeStore =
SegmentNodeStoreBuilders.builder(rwFileStore).build();
+ NodeBuilder builder = segmentNodeStore.getRoot().builder();
+
+
+ // simulate operation timeout when trying to renew lease
+ Mockito.reset(blobMocked);
+
+ StorageException storageException =
+ new
StorageException(StorageErrorCodeStrings.OPERATION_TIMED_OUT, "operation
timeout", new TimeoutException());
+
+
Mockito.doThrow(storageException).when(blobMocked).renewLease(Mockito.any(),
Mockito.any(), Mockito.any());
+
+
+ // wait till lease expires
+ Thread.sleep(70000);
Review Comment:
Now that we can configure the lease duration with the system property
`oak.segment.azure.lock.leaseDuration`, I would set it to the minimum of 15
seconds, so that we can reduce the sleep time here and make this test take less
time.
Probably we can do the same in `AzureRepositoryLockTests` as well.
##########
oak-segment-remote/src/main/java/org/apache/jackrabbit/oak/segment/remote/WriteAccessController.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.segment.remote;
+
+public class WriteAccessController {
+ private volatile boolean isWritingAllowed = false;
+
+ public void disableWriting() {
+ this.isWritingAllowed = false;
+ }
+
+ public void enableWriting() {
+ this.isWritingAllowed = true;
+
+ synchronized (this) {
+ this.notifyAll();
Review Comment:
Nitpick: even though the risk here is small, it's generally best practice to
avoid locking on `this`. I suggest using a `private final Object lock = new
Object();`
##########
oak-segment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLockTest.java:
##########
@@ -59,9 +58,9 @@ public void setup() throws StorageException,
InvalidKeyException, URISyntaxExcep
@Test
Review Comment:
Suggestion: add a test to validate the behaviour where it blocks writes
after several renewals have failed.
##########
oak-segment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureArchiveManagerTest.java:
##########
@@ -465,6 +475,80 @@ public void
testCollectBlobReferencesDoesNotFailWhenFileIsMissing() throws URISy
}
}
+ @Test
+ public void testWriteAfterLoosingRepoLock() throws URISyntaxException,
InvalidFileStoreVersionException, IOException, CommitFailedException,
StorageException, InterruptedException {
Review Comment:
polish: I suggest to replace the long list of exceptions in the throws
clause, by just `throws Exception`.
##########
oak-segment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureArchiveManagerTest.java:
##########
@@ -465,6 +475,80 @@ public void
testCollectBlobReferencesDoesNotFailWhenFileIsMissing() throws URISy
}
}
+ @Test
+ public void testWriteAfterLoosingRepoLock() throws URISyntaxException,
InvalidFileStoreVersionException, IOException, CommitFailedException,
StorageException, InterruptedException {
Review Comment:
Typo: should be `losing`
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
