cecemei commented on code in PR #19501: URL: https://github.com/apache/druid/pull/19501#discussion_r3344782103
########## processing/src/main/java/org/apache/druid/common/config/ConfigEtag.java: ########## @@ -0,0 +1,88 @@ +/* + * 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.druid.common.config; + +import javax.annotation.Nullable; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Arrays; +import java.util.Base64; + +/** + * Computes and compares ETags for stored dynamic config payloads. The ETag is a + * pure function of the payload bytes — used for {@code If-Match} preconditions. + */ +public final class ConfigEtag +{ + private static final int ETAG_HASH_BYTES = 16; + + private ConfigEtag() + { + } + + /** + * Quoted ETag for the given payload bytes, or {@code null} if {@code bytes} + * is {@code null}. SHA-256 truncated to {@value #ETAG_HASH_BYTES} bytes, + * base64url-encoded (no padding), wrapped in double quotes per RFC 7232. + */ + @Nullable + 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); + } + } + + /** + * 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) + { + if (ifMatchHeader == null) { + return true; + } + final String trimmed = ifMatchHeader.trim(); + if ("*".equals(trimmed)) { Review Comment: why does it need * for match? instead of not setting if match header. ########## processing/src/main/java/org/apache/druid/common/config/ConfigEtag.java: ########## @@ -0,0 +1,88 @@ +/* + * 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.druid.common.config; + +import javax.annotation.Nullable; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Arrays; +import java.util.Base64; + +/** + * Computes and compares ETags for stored dynamic config payloads. The ETag is a + * pure function of the payload bytes — used for {@code If-Match} preconditions. + */ +public final class ConfigEtag +{ + private static final int ETAG_HASH_BYTES = 16; + + private ConfigEtag() + { + } + + /** + * Quoted ETag for the given payload bytes, or {@code null} if {@code bytes} + * is {@code null}. SHA-256 truncated to {@value #ETAG_HASH_BYTES} bytes, + * base64url-encoded (no padding), wrapped in double quotes per RFC 7232. + */ + @Nullable + 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); + } + } + + /** + * 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) Review Comment: nit: `@Nullable String ifMatchHeader` ########## server/src/main/java/org/apache/druid/server/coordinator/CoordinatorConfigManager.java: ########## @@ -87,15 +88,62 @@ public CoordinatorDynamicConfig getCurrentDynamicConfig() return Preconditions.checkNotNull(dynamicConfig, "Got null config from watcher?!"); } + public CoordinatorDynamicConfig convertBytesToDynamicConfig(@Nullable byte[] bytes) + { + return jacksonConfigManager.convertByteToConfig( + bytes, + CoordinatorDynamicConfig.class, + CoordinatorDynamicConfig.builder().build() Review Comment: is it necessary to pass in a default value here? would it be used to compute for ETag? ########## server/src/main/java/org/apache/druid/server/coordinator/CoordinatorConfigManager.java: ########## @@ -268,14 +376,17 @@ public List<DataSourceCompactionConfigAuditEntry> getCompactionConfigHistory( private boolean updateConfigHelper( UnaryOperator<DruidCompactionConfig> configUpdateOperator, + @Nullable String ifMatchEtag, AuditInfo auditInfo ) { int attemps = 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; Review Comment: This feels strange, update could fail for other reasons too, maybe we dont want it to retry in these cases. the logic seems strange if Etag doesn't match but in line390 setResult is retryable. ########## processing/src/main/java/org/apache/druid/common/config/ConfigManager.java: ########## @@ -259,10 +283,11 @@ private MetadataCASUpdate createMetadataCASUpdate( public static class SetResult { - private static final SetResult SUCCESS = new SetResult(null, false); + private static final SetResult SUCCESS = new SetResult(null, false, false); private final Exception exception; private final boolean retryableException; + private final boolean preconditionFailed; Review Comment: The two-boolean design has an implicit invalid state (retryableException=true, preconditionFailed=true) and the semantics overlap in a confusing way, maybe consider use enum to represent the failure state. ``` - RETRYABLE_FAILURE — caller can re-read the current value and retry automatically (CAS conflict, concurrent writer) - PRECONDITION_FAILED — caller needs external input before retrying (stale ETag, requires client to re-fetch and resubmit — HTTP 412) - FAILURE — neither ``` ########## server/src/main/java/org/apache/druid/server/coordinator/CoordinatorConfigManager.java: ########## @@ -87,15 +88,62 @@ public CoordinatorDynamicConfig getCurrentDynamicConfig() return Preconditions.checkNotNull(dynamicConfig, "Got null config from watcher?!"); } + public CoordinatorDynamicConfig convertBytesToDynamicConfig(@Nullable byte[] bytes) + { + return jacksonConfigManager.convertByteToConfig( + bytes, + CoordinatorDynamicConfig.class, + CoordinatorDynamicConfig.builder().build() + ); + } + public ConfigManager.SetResult setDynamicConfig(CoordinatorDynamicConfig config, AuditInfo auditInfo) { - return jacksonConfigManager.set( + return setDynamicConfig(config, null, auditInfo); + } + + public ConfigManager.SetResult setDynamicConfig( Review Comment: there's no usage on `setDynamicConfig` any more with changes in this pr? ########## processing/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java: ########## @@ -127,6 +128,112 @@ public <T> SetResult set( return configManager.set(key, configSerde, oldValue, newValue); } + @Nullable + public byte[] getCurrentBytes(String key) + { + return configManager.getCurrentBytes(key); + } + + public boolean isCompareAndSwapEnabled() + { + return configManager.isCompareAndSwapEnabled(); + } + + /** + * Set the config, optionally guarded by an {@code If-Match}-style + * precondition. When {@code ifMatchEtag} is {@code null}, behaves like + * {@link #set(String, Object, AuditInfo)}. Otherwise the write only commits + * if the currently stored bytes hash to {@code ifMatchEtag}; on mismatch the + * result reports {@link SetResult#isPreconditionFailed() preconditionFailed}. + * + * <p>The precondition is enforced via metadata-store CAS, so conditional + * writes require {@code druid.manager.config.enableCompareAndSwap} to be + * true (the default). With CAS disabled, {@code If-Match} writes fail as a + * precondition failure instead of silently degrading to last-writer-wins. + */ + public <T> SetResult setIfMatch( + String key, + @Nullable String ifMatchEtag, + T newValue, + AuditInfo auditInfo + ) + { + if (newValue == null) { + return SetResult.failure(new IllegalArgumentException("input obj is null")); + } + if (ifMatchEtag == null) { + return set(key, newValue, auditInfo); + } + if (!configManager.isCompareAndSwapEnabled()) { + return SetResult.preconditionFailed( + new IllegalStateException( + "If-Match requires druid.manager.config.enableCompareAndSwap to be enabled for key[" + key + "]" + ) + ); + } + final byte[] currentBytes = configManager.getCurrentBytes(key); + if (!ConfigEtag.matches(ifMatchEtag, currentBytes)) { + return SetResult.preconditionFailed( + 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 + "]") + ); + } Review Comment: This seems hacky, we should have that check/logic in `set(key, currentBytes, newValue, auditInfo)`, maybe pass a retryableIfCurrentBytesNotMatch boolean, or maybe return a stale value failure and let caller decides what to do. ########## processing/src/main/java/org/apache/druid/common/config/ConfigEtag.java: ########## @@ -0,0 +1,88 @@ +/* + * 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.druid.common.config; + +import javax.annotation.Nullable; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Arrays; +import java.util.Base64; + +/** + * Computes and compares ETags for stored dynamic config payloads. The ETag is a + * pure function of the payload bytes — used for {@code If-Match} preconditions. + */ +public final class ConfigEtag +{ + private static final int ETAG_HASH_BYTES = 16; + + private ConfigEtag() + { + } + + /** + * Quoted ETag for the given payload bytes, or {@code null} if {@code bytes} + * is {@code null}. SHA-256 truncated to {@value #ETAG_HASH_BYTES} bytes, + * base64url-encoded (no padding), wrapped in double quotes per RFC 7232. + */ + @Nullable + public static String compute(@Nullable byte[] bytes) + { + if (bytes == null) { Review Comment: imho it'd nice to use non null value maybe `__NULL__ `to represent that, WDYT? ########## server/src/main/java/org/apache/druid/server/coordinator/CoordinatorConfigManager.java: ########## @@ -173,16 +254,25 @@ public boolean updateCompactionTaskSlots( return current.withClusterConfig(updatedClusterConfig); }; - return updateConfigHelper(operator, auditInfo); + return updateConfigHelper(operator, ifMatchEtag, auditInfo); + } + + public boolean updateClusterCompactionConfig( Review Comment: there's no usage in this function after this pr. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
