From 1bba8839f666a6600379c38fc90765c8230f2eab Mon Sep 17 00:00:00 2001 From: Lukas Krecan Date: Fri, 3 Jan 2025 20:17:12 +0100 Subject: [PATCH] S3 cleanup --- .../provider/s3/S3StorageAccessor.java | 50 +++++++++++-------- .../s3/S3LockProviderIntegrationTest.java | 2 +- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/providers/s3/shedlock-provider-s3/src/main/java/net/javacrumbs/shedlock/provider/s3/S3StorageAccessor.java b/providers/s3/shedlock-provider-s3/src/main/java/net/javacrumbs/shedlock/provider/s3/S3StorageAccessor.java index 4ef31db3a..b9a4a0a0b 100644 --- a/providers/s3/shedlock-provider-s3/src/main/java/net/javacrumbs/shedlock/provider/s3/S3StorageAccessor.java +++ b/providers/s3/shedlock-provider-s3/src/main/java/net/javacrumbs/shedlock/provider/s3/S3StorageAccessor.java @@ -1,15 +1,17 @@ package net.javacrumbs.shedlock.provider.s3; +import static net.javacrumbs.shedlock.core.ClockProvider.now; + import com.amazonaws.AmazonServiceException; import com.amazonaws.services.s3.AmazonS3; import com.amazonaws.services.s3.model.ObjectMetadata; import com.amazonaws.services.s3.model.PutObjectRequest; import com.amazonaws.services.s3.model.PutObjectResult; import java.io.ByteArrayInputStream; +import java.nio.ByteBuffer; import java.time.Instant; import java.util.Optional; import java.util.UUID; -import net.javacrumbs.shedlock.core.ClockProvider; import net.javacrumbs.shedlock.core.LockConfiguration; import net.javacrumbs.shedlock.support.AbstractStorageAccessor; @@ -17,11 +19,12 @@ * Implementation of StorageAccessor for S3 as a lock storage backend. * Manages locks using S3 objects with metadata for expiration and conditional writes. */ -public class S3StorageAccessor extends AbstractStorageAccessor { +class S3StorageAccessor extends AbstractStorageAccessor { private static final String LOCK_UNTIL = "lockUntil"; private static final String LOCKED_AT = "lockedAt"; private static final String LOCKED_BY = "lockedBy"; + private static final int PRECONDITION_FAILED = 412; private final AmazonS3 s3Client; private final String bucketName; @@ -58,15 +61,10 @@ Optional find(String name, String action) { @Override public boolean insertRecord(LockConfiguration lockConfiguration) { String name = lockConfiguration.getName(); - if (find(name, "insertRecord").isPresent()) { - logger.debug("Lock already exists. name: {}", name); - return false; - } try { - var lockContent = UUID.randomUUID().toString().getBytes(); - ObjectMetadata metadata = - createMetadata(lockConfiguration.getLockAtMostUntil(), ClockProvider.now(), getHostname()); + var lockContent = getLockContent(); + ObjectMetadata metadata = createMetadata(lockConfiguration.getLockAtMostUntil(), now(), getHostname()); metadata.setContentLength(lockContent.length); PutObjectRequest request = @@ -77,7 +75,7 @@ public boolean insertRecord(LockConfiguration lockConfiguration) { logger.debug("Lock created successfully. name: {}, metadata: {}", name, metadata.getUserMetadata()); return true; } catch (AmazonServiceException e) { - if (e.getStatusCode() == 412) { + if (e.getStatusCode() == PRECONDITION_FAILED) { logger.debug("Lock already in use. name: {}", name); } else { logger.warn("Failed to create lock. name: {}", name, e); @@ -89,16 +87,16 @@ public boolean insertRecord(LockConfiguration lockConfiguration) { @Override public boolean updateRecord(LockConfiguration lockConfiguration) { Optional lock = find(lockConfiguration.getName(), "updateRecord"); - if (lock.isEmpty() || lock.get().lockUntil().isAfter(ClockProvider.now())) { - logger.debug( - "Update skipped. Lock still valid or not found. name: {}, lock: {}", - lockConfiguration.getName(), - lock); + if (lock.isEmpty()) { + logger.warn("Update skipped. Lock not found. name: {}, lock: {}", lockConfiguration.getName(), lock); + return false; + } + if (lock.get().lockUntil().isAfter(now())) { + logger.debug("Update skipped. Lock still valid. name: {}, lock: {}", lockConfiguration.getName(), lock); return false; } - ObjectMetadata newMetadata = - createMetadata(lockConfiguration.getLockAtMostUntil(), ClockProvider.now(), getHostname()); + ObjectMetadata newMetadata = createMetadata(lockConfiguration.getLockAtMostUntil(), now(), getHostname()); return replaceObjectMetadata( lockConfiguration.getName(), newMetadata, lock.get().eTag(), "updateRecord"); } @@ -107,7 +105,7 @@ public boolean updateRecord(LockConfiguration lockConfiguration) { public void unlock(LockConfiguration lockConfiguration) { Optional lock = find(lockConfiguration.getName(), "unlock"); if (lock.isEmpty()) { - logger.debug("Unlock skipped. Lock not found. name: {}, lock: {}", lockConfiguration.getName(), lock); + logger.warn("Unlock skipped. Lock not found. name: {}, lock: {}", lockConfiguration.getName(), lock); return; } @@ -118,7 +116,7 @@ public void unlock(LockConfiguration lockConfiguration) { public boolean extend(LockConfiguration lockConfiguration) { Optional lock = find(lockConfiguration.getName(), "extend"); if (lock.isEmpty() - || lock.get().lockUntil().isBefore(ClockProvider.now()) + || lock.get().lockUntil().isBefore(now()) || !lock.get().lockedBy().equals(getHostname())) { logger.debug( "Extend skipped. Lock invalid or not owned by host. name: {}, lock: {}", @@ -139,7 +137,7 @@ private boolean updateUntil(String name, Lock lock, Instant until, String action } private boolean replaceObjectMetadata(String name, ObjectMetadata newMetadata, String eTag, String action) { - var lockContent = UUID.randomUUID().toString().getBytes(); + var lockContent = getLockContent(); newMetadata.setContentLength(lockContent.length); PutObjectRequest request = @@ -156,15 +154,23 @@ private boolean replaceObjectMetadata(String name, ObjectMetadata newMetadata, S response.getETag()); return true; } catch (AmazonServiceException e) { - if (e.getStatusCode() == 412) { + if (e.getStatusCode() == PRECONDITION_FAILED) { logger.debug("Lock not exists to {}. name: {}, e-tag {}", action, name, eTag); } else { - logger.warn("Failed to create lock. name: {}", name, e); + logger.warn("Failed to {} lock. name: {}", action, name, e); } return false; } } + private static byte[] getLockContent() { + var uuid = UUID.randomUUID(); + ByteBuffer bb = ByteBuffer.wrap(new byte[16]); + bb.putLong(uuid.getMostSignificantBits()); + bb.putLong(uuid.getLeastSignificantBits()); + return bb.array(); + } + private ObjectMetadata createMetadata(Instant lockUntil, Instant lockedAt, String lockedBy) { ObjectMetadata metadata = new ObjectMetadata(); metadata.addUserMetadata(LOCK_UNTIL, lockUntil.toString()); diff --git a/providers/s3/shedlock-provider-s3/src/test/java/net/javacrumbs/shedlock/provider/s3/S3LockProviderIntegrationTest.java b/providers/s3/shedlock-provider-s3/src/test/java/net/javacrumbs/shedlock/provider/s3/S3LockProviderIntegrationTest.java index f310f656a..a658eaa71 100644 --- a/providers/s3/shedlock-provider-s3/src/test/java/net/javacrumbs/shedlock/provider/s3/S3LockProviderIntegrationTest.java +++ b/providers/s3/shedlock-provider-s3/src/test/java/net/javacrumbs/shedlock/provider/s3/S3LockProviderIntegrationTest.java @@ -28,7 +28,7 @@ public class S3LockProviderIntegrationTest extends AbstractStorageBasedLockProviderIntegrationTest { @Container - public static final MyLocalStackS3Container localStackS3 = new MyLocalStackS3Container(); + static final MyLocalStackS3Container localStackS3 = new MyLocalStackS3Container(); private static AmazonS3 s3Client; private static final String BUCKET_NAME = "my-bucket";