more stuff

Author: jtuglu1

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

@@ -65,7 +65,7 @@ public static String compute(@Nullable byte[] bytes)
    * 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 + "]")
       );

server/src/main/java/org/apache/druid/server/coordinator/CoordinatorConfigManager.java

@@ -97,25 +97,6 @@ public CoordinatorDynamicConfig convertBytesToDynamicConfig(@Nullable byte[] byt
     );
   }
 
-  public ConfigManager.SetResult setDynamicConfig(CoordinatorDynamicConfig config, AuditInfo auditInfo)
-  {
-    return setDynamicConfig(config, null, auditInfo);
-  }
-
-  public ConfigManager.SetResult setDynamicConfig(
-      CoordinatorDynamicConfig config,
-      @Nullable String ifMatchEtag,
-      AuditInfo auditInfo
-  )
-  {
-    return jacksonConfigManager.setIfMatch(
-        CoordinatorDynamicConfig.CONFIG_KEY,
-        ifMatchEtag,
-        config,
-        auditInfo
-    );
-  }
-
   public ConfigManager.SetResult updateDynamicConfig(
       UnaryOperator<CoordinatorDynamicConfig> operator,
       @Nullable String ifMatchEtag,
@@ -205,14 +186,22 @@ public ConfigManager.SetResult getAndUpdateCompactionConfig(
 
     if (current.equals(updated)) {
       return ConfigManager.SetResult.ok();
-    } else {
-      return jacksonConfigManager.set(
-          DruidCompactionConfig.CONFIG_KEY,
-          currentBytes,
-          updated,
-          auditInfo
+    }
+    final ConfigManager.SetResult result = jacksonConfigManager.set(
+        DruidCompactionConfig.CONFIG_KEY,
+        currentBytes,
+        updated,
+        auditInfo
+    );
+    // Under If-Match, a lost CAS means a concurrent writer committed between our
+    // read and write: the precondition no longer holds, so report it as such
+    // rather than as a retryable failure (conditional writes must not auto-retry).
+    if (ifMatchEtag != null && result.isRetryable()) {
+      return ConfigManager.SetResult.preconditionFailed(
+          new IllegalStateException("If-Match precondition failed (concurrent update) for compaction config")
       );
     }
+    return result;
   }
 
   public DruidCompactionConfig convertBytesToCompactionConfig(@Nullable byte[] bytes)
@@ -257,14 +246,6 @@ public boolean updateCompactionTaskSlots(
     return updateConfigHelper(operator, ifMatchEtag, auditInfo);
   }
 
-  public boolean updateClusterCompactionConfig(
-      ClusterCompactionConfig config,
-      AuditInfo auditInfo
-  )
-  {
-    return updateClusterCompactionConfig(config, null, auditInfo);
-  }
-
   public boolean updateClusterCompactionConfig(
       ClusterCompactionConfig config,
       @Nullable String ifMatchEtag,
@@ -380,17 +361,18 @@ private boolean updateConfigHelper(
       AuditInfo auditInfo
   )
   {
-    int attemps = 0;
+    int attempts = 0;
     ConfigManager.SetResult setResult = null;
-    // When the caller has supplied an If-Match precondition, do not retry on CAS failure.
-    final int maxAttempts = ifMatchEtag != null ? 1 : MAX_UPDATE_RETRIES;
+    // Only an unconditional write can be retried: getAndUpdateCompactionConfig converts a lost
+    // CAS under If-Match into a (non-retryable) precondition failure, so conditional writes exit
+    // this loop on the first attempt rather than silently retrying past the caller's precondition.
     try {
-      while (attemps < maxAttempts) {
+      while (attempts < MAX_UPDATE_RETRIES) {
         setResult = getAndUpdateCompactionConfig(configUpdateOperator, ifMatchEtag, auditInfo);
         if (setResult.isOk() || !setResult.isRetryable()) {
           break;
         }
-        attemps++;
+        attempts++;
         updateRetryDelay();
       }
     }
@@ -409,18 +391,14 @@ private boolean updateConfigHelper(
       return true;
     }
 
-    // With If-Match, a retryable CAS failure means a concurrent writer beat us
-    // between the precondition check and the CAS — surface as 412 too.
-    final boolean preconditionFailed = setResult.isPreconditionFailed()
-                                       || (ifMatchEtag != null && setResult.isRetryable());
-    if (preconditionFailed) {
+    // getAndUpdateCompactionConfig already converts a lost CAS under If-Match into
+    // a precondition failure, so a retryable result here is only a genuine
+    // (non-conditional) CAS conflict that exhausted its retries.
+    if (setResult.isPreconditionFailed()) {
       log.info("If-Match precondition failed on compaction config update");
-      final String message = setResult.isPreconditionFailed()
-          ? setResult.getException().getMessage()
-          : "If-Match precondition failed (concurrent update) on compaction config";
       throw DruidException.forPersona(DruidException.Persona.USER)
                           .ofCategory(DruidException.Category.PRECONDITION_FAILED)
-                          .build(message);
+                          .build(setResult.getException().getMessage());
     }
 
     if (setResult.getException() instanceof NoSuchElementException) {