Merge pull request #19962 from vespa-engine/revert-19960-revert-19957-hmusum/always-set-new-config-when-updated-generation-take-2v7.499.15

Always set new config when updated generation, take 3

3 files changed, 39 insertions, 2 deletions

diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSubscription.java
index 6a81c2279d1..a3265671d50 100644
--- a/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSubscription.java
+++ b/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSubscription.java
@@ -16,7 +16,9 @@ import com.yahoo.vespa.config.TimingValues;
 import com.yahoo.vespa.config.protocol.DefContent;
 
 import java.io.File;
+import java.util.Objects;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.vespa.config.PayloadChecksum.Type.MD5;
@@ -203,6 +205,18 @@ public abstract class ConfigSubscription<T extends ConfigInstance> {
         this.config.set(new ConfigState<>(true, generation, applyOnRestart, true, config, payloadChecksums));
     }
 
+    void setConfigAndGeneration(Long generation, boolean applyOnRestart, T config, PayloadChecksums payloadChecksums) {
+        ConfigState<T> prev = this.config.get();
+        boolean configChanged = !Objects.equals(prev.getConfig(), config);
+        String message = "Config has changed unexpectedly for " + key + ", generation " + generation;
+        if (configChanged) {
+            if (log.isLoggable(Level.FINE))
+                message = message + ", config in state :" + prev.getConfig() + ", new config: " + config;
+            log.log(Level.WARNING, message);
+        }
+        this.config.set(new ConfigState<>(true, generation, applyOnRestart, configChanged, config, payloadChecksums));
+    }
+
     /**
      * Used by {@link FileConfigSubscription} and {@link ConfigSetSubscription}
      */
@@ -213,7 +227,7 @@ public abstract class ConfigSubscription<T extends ConfigInstance> {
 
     protected void setConfigIfChanged(T config) {
         ConfigState<T> prev = this.config.get();
-        this.config.set(new ConfigState<>(true, prev.getGeneration(), prev.applyOnRestart(), !config.equals(prev.getConfig()), config, prev.payloadChecksums));
+        this.config.set(new ConfigState<>(true, prev.getGeneration(), prev.applyOnRestart(), !Objects.equals(prev.getConfig(), config), config, prev.payloadChecksums));
     }
 
     void setGeneration(Long generation) {
diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/GenericJRTConfigSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/GenericJRTConfigSubscription.java
index dbe13e90c9c..354489ea946 100644
--- a/config/src/main/java/com/yahoo/config/subscription/impl/GenericJRTConfigSubscription.java
+++ b/config/src/main/java/com/yahoo/config/subscription/impl/GenericJRTConfigSubscription.java
@@ -49,6 +49,17 @@ public class GenericJRTConfigSubscription extends JRTConfigSubscription<RawConfi
         }
     }
 
+    // Need to override this method, since we use RawConfig in this class,
+    @Override
+    protected void setNewConfigAndGeneration(JRTClientConfigRequest jrtReq) {
+        // Set generation first, as RawConfig contains generation and that
+        // will make configChanged in ConfigState always true otherwise
+        // (see equals usage in setConfigAndGeneration())
+        setGeneration(jrtReq.getNewGeneration());
+        RawConfig rawConfig = RawConfig.createFromResponseParameters(jrtReq);
+        setConfigAndGeneration(jrtReq.getNewGeneration(), jrtReq.responseIsApplyOnRestart(), rawConfig, jrtReq.getNewChecksums());
+    }
+
     // Override to propagate internal redeploy into the config value in addition to the config state
     @Override
     void setApplyOnRestart(boolean applyOnRestart) {
diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigSubscription.java
index 27099790f5b..c6ea79ddbcd 100644
--- a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigSubscription.java
+++ b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigSubscription.java
@@ -73,7 +73,7 @@ public class JRTConfigSubscription<T extends ConfigInstance> extends ConfigSubsc
             if (jrtReq.hasUpdatedConfig()) {
                 setNewConfig(jrtReq);
             } else {
-                setGeneration(jrtReq.getNewGeneration());
+                setNewConfigAndGeneration(jrtReq);
             }
         }
 
@@ -110,6 +110,18 @@ public class JRTConfigSubscription<T extends ConfigInstance> extends ConfigSubsc
         }
     }
 
+    protected void setNewConfigAndGeneration(JRTClientConfigRequest jrtReq) {
+        try {
+            T configInstance = toConfigInstance(jrtReq);
+            setConfigAndGeneration(jrtReq.getNewGeneration(),
+                                   jrtReq.responseIsApplyOnRestart(),
+                                   configInstance,
+                                   jrtReq.getNewChecksums());
+        } catch (IllegalArgumentException e) {
+            throw new IllegalArgumentException("Bad config in response", e);
+        }
+    }
+
     /**
      * This method should ideally throw new MissingConfig/Configuration exceptions and let the caller
      * catch them. However, this would make the code in JRT/File/RawSource uglier.