more stuff

Author: jtuglu1

indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResource.java

@@ -122,14 +122,19 @@ public Response updateClusterCompactionConfig(
       @Context HttpServletRequest req
   )
   {
-    final AuditInfo auditInfo = AuthorizationUtils.buildAuditInfo(req);
-    return ServletResourceUtils.buildUpdateResponse(
-        () -> configManager.updateClusterCompactionConfig(
-            updatePayload,
-            DynamicConfigEtagHelper.getIfMatch(req),
-            auditInfo
-        )
-    );
+    try {
+      final AuditInfo auditInfo = AuthorizationUtils.buildAuditInfo(req);
+      return DynamicConfigEtagHelper.buildSetResultUpdateResponse(
+          configManager.updateClusterCompactionConfig(
+              updatePayload,
+              DynamicConfigEtagHelper.getIfMatch(req),
+              auditInfo
+          )
+      );
+    }
+    catch (DruidException e) {
+      return ServletResourceUtils.buildErrorResponseFrom(e);
+    }
   }
 
   @GET

indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordCompactionResourceTest.java

@@ -24,6 +24,7 @@
 import org.apache.druid.audit.AuditManager;
 import org.apache.druid.client.coordinator.CoordinatorClient;
 import org.apache.druid.common.config.ConfigEtag;
+import org.apache.druid.common.config.ConfigManager;
 import org.apache.druid.error.DruidExceptionMatcher;
 import org.apache.druid.error.ErrorResponse;
 import org.apache.druid.indexer.CompactionEngine;
@@ -160,7 +161,7 @@ public void test_updateClusterConfig()
         EasyMock.anyObject(),
         EasyMock.anyObject()
     ))
-            .andReturn(true)
+            .andReturn(ConfigManager.SetResult.ok())
             .once();
 
     setupMockRequestForAudit();

processing/src/main/java/org/apache/druid/common/config/ConfigEtag.java

@@ -32,6 +32,14 @@
 public final class ConfigEtag
 {
   private static final int ETAG_HASH_BYTES = 16;
+  private static final ThreadLocal<MessageDigest> SHA_256 = ThreadLocal.withInitial(() -> {
+    try {
+      return MessageDigest.getInstance("SHA-256");
+    }
+    catch (NoSuchAlgorithmException e) {
+      throw new IllegalStateException("SHA-256 not available", e);
+    }
+  });
 
   private ConfigEtag()
   {
@@ -48,24 +56,19 @@ public static String compute(@Nullable byte[] bytes)
     if (bytes == null) {
       return null;
     }
-    try {
-      MessageDigest md = MessageDigest.getInstance("SHA-256");
-      byte[] full = md.digest(bytes);
-      byte[] truncated = Arrays.copyOf(full, ETAG_HASH_BYTES);
-      return "\"" + Base64.getUrlEncoder().withoutPadding().encodeToString(truncated) + "\"";
-    }
-    catch (NoSuchAlgorithmException e) {
-      // SHA-256 is required by every JRE.
-      throw new IllegalStateException("SHA-256 not available", e);
-    }
+    final MessageDigest md = SHA_256.get();
+    md.reset();
+    final byte[] full = md.digest(bytes);
+    final byte[] truncated = Arrays.copyOf(full, ETAG_HASH_BYTES);
+    return "\"" + Base64.getUrlEncoder().withoutPadding().encodeToString(truncated) + "\"";
   }
 
   /**
    * Whether {@code ifMatchHeader} matches the ETag of {@code currentBytes}.
    * Wildcard {@code *} matches any existing value. A comma-separated list is
    * satisfied if any element matches.
    */
-  public static boolean matches(String ifMatchHeader, @Nullable byte[] currentBytes)
+  public static boolean matches(@Nullable String ifMatchHeader, @Nullable byte[] currentBytes)
   {
     if (ifMatchHeader == null) {
       return true;

processing/src/main/java/org/apache/druid/common/config/ConfigManager.java

@@ -283,11 +283,27 @@ private MetadataCASUpdate createMetadataCASUpdate(
 
   public static class SetResult
   {
-    private static final SetResult SUCCESS = new SetResult(null, false, false);
-    private final Exception exception;
+    /**
+     * Outcome of a {@link #set} attempt. Mutually exclusive, so callers never
+     * have to reconcile overlapping flags.
+     */
+    private enum Status
+    {
+      /** The write committed. */
+      OK,
+      /** A concurrent writer won the CAS; the caller may re-read and retry. */
+      RETRYABLE,
+      /** A client-supplied precondition (e.g. {@code If-Match}) failed; maps to HTTP 412. */
+      PRECONDITION_FAILED,
+      /** A non-retryable failure (bad input, manager not started, etc.). */
+      FAILURE
+    }
 
-    private final boolean retryableException;
-    private final boolean preconditionFailed;
+    private static final SetResult SUCCESS = new SetResult(Status.OK, null);
+
+    private final Status status;
+    @Nullable
+    private final Exception exception;
 
     public static SetResult ok()
     {
@@ -296,42 +312,42 @@ public static SetResult ok()
 
     public static SetResult failure(Exception e)
     {
-      return new SetResult(e, false, false);
+      return new SetResult(Status.FAILURE, e);
     }
 
     public static SetResult retryableFailure(Exception e)
     {
-      return new SetResult(e, true, false);
+      return new SetResult(Status.RETRYABLE, e);
     }
 
     /** Client-supplied precondition (e.g. {@code If-Match}) failed; maps to HTTP 412. */
     public static SetResult preconditionFailed(Exception e)
     {
-      return new SetResult(e, false, true);
+      return new SetResult(Status.PRECONDITION_FAILED, e);
     }
 
-    private SetResult(@Nullable Exception exception, boolean retryableException, boolean preconditionFailed)
+    private SetResult(Status status, @Nullable Exception exception)
     {
+      this.status = status;
       this.exception = exception;
-      this.retryableException = retryableException;
-      this.preconditionFailed = preconditionFailed;
     }
 
     public boolean isOk()
     {
-      return exception == null;
+      return status == Status.OK;
     }
 
     public boolean isRetryable()
     {
-      return retryableException;
+      return status == Status.RETRYABLE;
     }
 
     public boolean isPreconditionFailed()
     {
-      return preconditionFailed;
+      return status == Status.PRECONDITION_FAILED;
     }
 
+    @Nullable
     public Exception getException()
     {
       return exception;

processing/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java

@@ -177,15 +177,7 @@ public <T> SetResult setIfMatch(
           new IllegalStateException("If-Match precondition failed for key[" + key + "]")
       );
     }
-    final SetResult result = set(key, currentBytes, newValue, auditInfo);
-    // Retryable CAS failure here = concurrent writer between our read and CAS;
-    // surface as precondition failed since the caller asked us to reject that.
-    if (!result.isOk() && result.isRetryable()) {
-      return SetResult.preconditionFailed(
-          new IllegalStateException("If-Match precondition failed (concurrent update) for key[" + key + "]")
-      );
-    }
-    return result;
+    return casConflictAsPreconditionFailed(set(key, currentBytes, newValue, auditInfo), key);
   }
 
   /**
@@ -225,8 +217,18 @@ public <T> SetResult setIfMatch(
     if (ifMatchEtag == null) {
       return set(key, newValue, auditInfo);
     }
-    final SetResult result = set(key, currentBytes, newValue, auditInfo);
-    if (!result.isOk() && result.isRetryable()) {
+    return casConflictAsPreconditionFailed(set(key, currentBytes, newValue, auditInfo), key);
+  }
+
+  /**
+   * Maps a CAS-conflict ({@link SetResult#isRetryable() retryable}) outcome to a
+   * precondition failure. A conditional write that loses the CAS means another
+   * writer committed between our read and write, so the supplied {@code If-Match}
+   * no longer describes the stored value — the caller must re-read and retry.
+   */
+  private static SetResult casConflictAsPreconditionFailed(SetResult result, String key)
+  {
+    if (result.isRetryable()) {
       return SetResult.preconditionFailed(
           new IllegalStateException("If-Match precondition failed (concurrent update) for key[" + key + "]")
       );

processing/src/test/java/org/apache/druid/common/config/JacksonConfigManagerTest.java

@@ -286,6 +286,35 @@ public void testSetIfMatchTransformPreconditionFailsWhenCompareAndSwapDisabled()
     Mockito.verify(mockAuditManager, Mockito.never()).doAudit(Mockito.any(AuditEntry.class));
   }
 
+  @Test
+  public void testSetIfMatchCasConflictReturnsPreconditionFailed()
+  {
+    final String key = "key";
+    final TestConfig val = new TestConfig("v", "s", 1);
+    final AuditInfo auditInfo = new AuditInfo("a", "i", "c", "ip");
+    final byte[] currentBytes = "current".getBytes(java.nio.charset.StandardCharsets.UTF_8);
+
+    Mockito.when(mockConfigManager.isCompareAndSwapEnabled()).thenReturn(true);
+    Mockito.when(mockConfigManager.getCurrentBytes(key)).thenReturn(currentBytes);
+    Mockito.when(mockConfigManager.set(
+        Mockito.eq(key),
+        Mockito.any(ConfigSerde.class),
+        Mockito.eq(currentBytes),
+        Mockito.eq(val)
+    )).thenReturn(ConfigManager.SetResult.retryableFailure(new IllegalStateException("Config value has changed")));
+
+    final ConfigManager.SetResult result = jacksonConfigManager.setIfMatch(
+        key,
+        ConfigEtag.compute(currentBytes),
+        val,
+        auditInfo
+    );
+
+    Assertions.assertFalse(result.isOk());
+    Assertions.assertTrue(result.isPreconditionFailed());
+    Assertions.assertFalse(result.isRetryable());
+  }
+
   @Test
   public void testConvertByteToConfigWithNullConfigInByte()
   {