You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2022/11/18 12:11:18 UTC
[GitHub] [jackrabbit-oak] jelmini opened a new pull request, #760: OAK-10006 Prevent writes to Azure storage if lock is not held
jelmini opened a new pull request, #760:
URL: https://github.com/apache/jackrabbit-oak/pull/760
If the lock is lost to another instance, all writes attempts should fail with an exception.
If the lock fails to be renewed, all writes are suspended (blocked) until the renewal succeed.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10006 Prevent writes to Azure storage if lock is not held [jackrabbit-oak]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #760:
URL: https://github.com/apache/jackrabbit-oak/pull/760#issuecomment-2599464489
This PR is stale because it has been open 24 months with no activity. Remove stale label or comment or this will be closed in 30 days.
--
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: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] jelmini commented on a diff in pull request #760: OAK-10006 Prevent writes to Azure storage if lock is not held
Posted by GitBox <gi...@apache.org>.
jelmini commented on code in PR #760:
URL: https://github.com/apache/jackrabbit-oak/pull/760#discussion_r1049651644
##########
oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java:
##########
@@ -237,6 +237,28 @@ private static GarbageCollectionStrategy newGarbageCollectionStrategy() {
log.debug("TAR files: {}", tarFiles);
}
+ private void onLockStatusChanged(SegmentNodeStorePersistence.LockStatus lockStatus) {
+ switch (lockStatus) {
+ case LOST:
+ try {
+ if (tarFiles != null) {
+ tarFiles.close();
Review Comment:
Right. Thanks for spotting this, @ahanikel !
I suppose we should add an `unsafeShutdown()` method to `TarFiles`, `TarWriter` and `AbstractRemoteSegmentArchiveWriter`, so that we can shut them down without attempting any finalisation writes to the remote storage, which would anyway fail or corrupt the data.
What do you think?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10006 Prevent writes to Azure storage if lock is not held [jackrabbit-oak]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #760: OAK-10006 Prevent writes to Azure storage if lock is not held
URL: https://github.com/apache/jackrabbit-oak/pull/760
--
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: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] ahanikel commented on a diff in pull request #760: OAK-10006 Prevent writes to Azure storage if lock is not held
Posted by GitBox <gi...@apache.org>.
ahanikel commented on code in PR #760:
URL: https://github.com/apache/jackrabbit-oak/pull/760#discussion_r1027810157
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java:
##########
@@ -121,6 +132,21 @@ private void refreshLease() {
}
}
+ private void notifyLockStatusChange(LockStatus renewal) {
+ try {
+ lockStatusChangedCallback.accept(renewal);
+ } catch (RuntimeException e) {
+ log.warn("Exception in lock status change callback", e);
+ }
+ }
+
+ void doRenewLease() throws StorageException {
+ BlobRequestOptions blobRequestOptions = new BlobRequestOptions();
+ blobRequestOptions.setMaximumExecutionTimeInMs(Math.max(INTERVAL - renewalInterval - 1, INTERVAL / 2 - 1));
Review Comment:
Could you add a comment here that explains this?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] ahanikel commented on a diff in pull request #760: OAK-10006 Prevent writes to Azure storage if lock is not held
Posted by GitBox <gi...@apache.org>.
ahanikel commented on code in PR #760:
URL: https://github.com/apache/jackrabbit-oak/pull/760#discussion_r1027835132
##########
oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java:
##########
@@ -237,6 +237,28 @@ private static GarbageCollectionStrategy newGarbageCollectionStrategy() {
log.debug("TAR files: {}", tarFiles);
}
+ private void onLockStatusChanged(SegmentNodeStorePersistence.LockStatus lockStatus) {
+ switch (lockStatus) {
+ case LOST:
+ try {
+ if (tarFiles != null) {
+ tarFiles.close();
Review Comment:
This will write the graph and references to the tar file (`archive.close()` in `TarWriter`), shouldn't we avoid that? Also it will try to flush the write queue (`q.flush()` in `AbstractRemoteSegmentArchiveWriter`), so if it's not empty, it will wait there forever, right?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] jelmini commented on a diff in pull request #760: OAK-10006 Prevent writes to Azure storage if lock is not held
Posted by GitBox <gi...@apache.org>.
jelmini commented on code in PR #760:
URL: https://github.com/apache/jackrabbit-oak/pull/760#discussion_r1049715995
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java:
##########
@@ -121,6 +132,21 @@ private void refreshLease() {
}
}
+ private void notifyLockStatusChange(LockStatus renewal) {
+ try {
+ lockStatusChangedCallback.accept(renewal);
+ } catch (RuntimeException e) {
+ log.warn("Exception in lock status change callback", e);
+ }
+ }
+
+ void doRenewLease() throws StorageException {
+ BlobRequestOptions blobRequestOptions = new BlobRequestOptions();
+ blobRequestOptions.setMaximumExecutionTimeInMs(Math.max(INTERVAL - renewalInterval - 1, INTERVAL / 2 - 1));
Review Comment:
Ok, I will add a comment.
In summary, previously `renewLease()` was called with the default `BlobRequestOptions`. However, those defaults can be set globally for the client instance and may be not a good fit for calling `renewLease()` as it could block for too long. As the lease is acquired for 60 seconds and `renewLease()` is called by default every 30 seconds, I think it should timeout after at most 29 seconds (giving 1 sec margin), so that we can call `notifyLockStatusChange(LockStatus.RENEWAL_FAILED)` and suspend writes before the lease becomes available to be acquired by another instance.
BTW, I have just noticed that values should be in millis and thus multiplied by 1000...
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org