diff options
17 files changed, 170 insertions, 179 deletions
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java index 528c61fe132..a52aa6d7216 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriber.java @@ -7,7 +7,6 @@ import com.yahoo.config.subscription.impl.GenericConfigHandle; import com.yahoo.config.subscription.impl.GenericConfigSubscriber; import com.yahoo.config.subscription.impl.JRTConfigRequester; import com.yahoo.log.LogLevel; -import com.yahoo.vespa.config.ConfigKey; import com.yahoo.yolean.Exceptions; import com.yahoo.vespa.config.RawConfig; import com.yahoo.vespa.config.TimingValues; @@ -31,10 +30,12 @@ public class UpstreamConfigSubscriber implements Subscriber { private GenericConfigSubscriber subscriber; private GenericConfigHandle handle; - UpstreamConfigSubscriber(RawConfig config, ClientUpdater clientUpdater, ConfigSource configSourceSet, - TimingValues timingValues, Map<ConfigSourceSet, JRTConfigRequester> requesterPool, - MemoryCache memoryCache) - { + UpstreamConfigSubscriber(RawConfig config, + ClientUpdater clientUpdater, + ConfigSource configSourceSet, + TimingValues timingValues, + Map<ConfigSourceSet, JRTConfigRequester> requesterPool, + MemoryCache memoryCache) { this.config = config; this.clientUpdater = clientUpdater; this.configSourceSet = configSourceSet; @@ -45,9 +46,7 @@ public class UpstreamConfigSubscriber implements Subscriber { void subscribe() { subscriber = new GenericConfigSubscriber(requesterPool); - ConfigKey<?> key = config.getKey(); - handle = subscriber.subscribe(new ConfigKey<RawConfig>(key.getName(), key.getConfigId(), key.getNamespace()), - config.getDefContent(), configSourceSet, timingValues); + handle = subscriber.subscribe(config.getKey(), config.getDefContent(), configSourceSet, timingValues); } @Override diff --git a/config/src/main/java/com/yahoo/config/subscription/ConfigHandle.java b/config/src/main/java/com/yahoo/config/subscription/ConfigHandle.java index bb29e329121..6b8059ad5af 100644 --- a/config/src/main/java/com/yahoo/config/subscription/ConfigHandle.java +++ b/config/src/main/java/com/yahoo/config/subscription/ConfigHandle.java @@ -52,7 +52,7 @@ public class ConfigHandle<T extends ConfigInstance> { */ public T getConfig() { // TODO throw if subscriber not frozen? - return sub.getConfigState().getConfig(); + return sub.getConfig(); } @Override diff --git a/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java b/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java index bf8b9372d58..1c9f659b45f 100644 --- a/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java +++ b/config/src/main/java/com/yahoo/config/subscription/ConfigSubscriber.java @@ -230,7 +230,7 @@ public class ConfigSubscriber { boolean anyConfigChanged = false; boolean allGenerationsChanged = true; boolean allGenerationsTheSame = true; - Long currentGen = null; + Long currentGenChecker = null; for (ConfigHandle<? extends ConfigInstance> h : subscriptionHandles) { h.setChanged(false); // Reset this flag, if it was set, the user should have acted on it the last time this method returned true. } @@ -244,11 +244,10 @@ public class ConfigSubscriber { return false; } throwIfExceptionSet(subscription); - ConfigSubscription.ConfigState<? extends ConfigInstance> config = subscription.getConfigState(); - if (currentGen == null) currentGen = config.getGeneration(); - if (!currentGen.equals(config.getGeneration())) allGenerationsTheSame = false; - allGenerationsChanged = allGenerationsChanged && config.isGenerationChanged(); - if (config.isConfigChanged()) anyConfigChanged = true; + if (currentGenChecker == null) currentGenChecker = subscription.getGeneration(); + if (!currentGenChecker.equals(subscription.getGeneration())) allGenerationsTheSame = false; + allGenerationsChanged = allGenerationsChanged && subscription.isGenerationChanged(); + if (subscription.isConfigChanged()) anyConfigChanged = true; timeLeftMillis = timeLeftMillis - (System.currentTimeMillis() - started); } reconfigDue = (anyConfigChanged || !requireChange) && allGenerationsChanged && allGenerationsTheSame; @@ -259,8 +258,8 @@ public class ConfigSubscriber { if (reconfigDue) { // This indicates the clients will possibly reconfigure their services, so "reset" changed-logic in subscriptions. // Also if appropriate update the changed flag on the handler, which clients use. - markSubsChangedSeen(currentGen); - generation = currentGen; + markSubsChangedSeen(); + generation = subscriptionHandles.get(0).subscription().getGeneration(); } return reconfigDue; } @@ -286,10 +285,11 @@ public class ConfigSubscriber { } } - private void markSubsChangedSeen(Long requiredGen) { + private void markSubsChangedSeen() { for (ConfigHandle<? extends ConfigInstance> h : subscriptionHandles) { ConfigSubscription<? extends ConfigInstance> sub = h.subscription(); - h.setChanged(sub.isConfigChangedAndReset(requiredGen)); + h.setChanged(sub.isConfigChanged()); + sub.resetChangedFlags(); } } diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSetSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSetSubscription.java index acd5b823af3..b39ae9e93cc 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSetSubscription.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/ConfigSetSubscription.java @@ -35,19 +35,22 @@ public class ConfigSetSubscription<T extends ConfigInstance> extends ConfigSubsc public boolean nextConfig(long timeout) { long end = System.currentTimeMillis() + timeout; do { - T myInstance = getNewInstance(); - ConfigState<T> configState = getConfigState(); + ConfigInstance myInstance = getNewInstance(); // User forced reload if (checkReloaded()) { - setConfigIfChanged(myInstance); + updateInstance(myInstance); return true; } - if (!myInstance.equals(configState.getConfig())) { - setConfigIfChangedIncGen(myInstance); + if (!myInstance.equals(config)) { + generation++; + updateInstance(myInstance); return true; } sleep(); } while (System.currentTimeMillis() < end); + // These shouldn't be checked anywhere since we return false now, but setting them still + setGenerationChanged(false); + setConfigChanged(false); return false; } @@ -59,17 +62,25 @@ public class ConfigSetSubscription<T extends ConfigInstance> extends ConfigSubsc } } + @SuppressWarnings("unchecked") + private void updateInstance(ConfigInstance myInstance) { + if (!myInstance.equals(config)) { + setConfigChanged(true); + } + setConfig((T) myInstance); + setGenerationChanged(true); + } + @Override public boolean subscribe(long timeout) { return true; } - @SuppressWarnings("unchecked") - private T getNewInstance() { + public ConfigInstance getNewInstance() { try { ConfigInstance.Builder builder = set.get(subKey); Constructor<?> constructor = builder.getClass().getDeclaringClass().getConstructor(builder.getClass()); - return (T) constructor.newInstance(builder); + return (ConfigInstance) constructor.newInstance(builder); } catch (Exception e) { e.printStackTrace(); throw new RuntimeException(e); 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 02c455bf1c3..398392ba8d3 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 @@ -2,8 +2,6 @@ package com.yahoo.config.subscription.impl; import java.io.File; -import java.util.concurrent.Callable; -import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; import com.yahoo.config.ConfigInstance; @@ -27,43 +25,22 @@ import com.yahoo.vespa.config.protocol.DefContent; */ public abstract class ConfigSubscription<T extends ConfigInstance> { protected static Logger log = Logger.getLogger(ConfigSubscription.class.getName()); - protected final ConfigSubscriber subscriber; - private final AtomicReference<ConfigState<T>> config = new AtomicReference<>(); - protected final ConfigKey<T> key; - protected final Class<T> configClass; + protected ConfigSubscriber subscriber; + protected boolean configChanged = false; + protected boolean generationChanged = false; + protected volatile T config = null; + protected Long generation = null; + protected ConfigKey<T> key; + protected Class<T> configClass; private volatile RuntimeException exception = null; private State state = State.OPEN; - - public static class ConfigState<T extends ConfigInstance> { - private final boolean configChanged; - private final boolean generationChanged; - private final T config; - private final Long generation; - private ConfigState(boolean generationChanged, Long generation, boolean configChanged, T config) { - this.configChanged = configChanged; - this.config = config; - this.generationChanged = generationChanged; - this.generation = generation; - } - private ConfigState(Long generation, T config) { - this(false, generation,false, config); - } - private ConfigState() { - this(false, 0L, false, null); - } - private ConfigState<T> createUnchanged() { return new ConfigState<T>(generation, config); } - public boolean isConfigChanged() { return configChanged; } - public boolean isGenerationChanged() { return generationChanged; } - public Long getGeneration() { return generation; } - public T getConfig() { return config; } - } /** * If non-null: The user has set this generation explicitly. nextConfig should take this into account. * Access to these variables _must_ be synchronized, as nextConfig and reload() is likely to be run from * independent threads. */ - - private final AtomicReference<Long> reloadedGeneration = new AtomicReference<>(); + private boolean doReload = false; + private long reloadedGeneration = -1; enum State { OPEN, CLOSED @@ -79,7 +56,6 @@ public abstract class ConfigSubscription<T extends ConfigInstance> { this.key = key; this.configClass = key.getConfigClass(); this.subscriber = subscriber; - this.config.set(new ConfigState<T>()); } @@ -90,8 +66,7 @@ public abstract class ConfigSubscription<T extends ConfigInstance> { * @param subscriber the subscriber for this subscription * @return a subclass of a ConfigsSubscription */ - public static <T extends ConfigInstance> ConfigSubscription<T> get(ConfigKey<T> key, ConfigSubscriber subscriber, - ConfigSource source, TimingValues timingValues) { + public static <T extends ConfigInstance> ConfigSubscription<T> get(ConfigKey<T> key, ConfigSubscriber subscriber, ConfigSource source, TimingValues timingValues) { String configId = key.getConfigId(); if (source instanceof RawSource || configId.startsWith("raw:")) return getRawSub(key, subscriber, source); if (source instanceof FileSource || configId.startsWith("file:")) return getFileSub(key, subscriber, source); @@ -154,54 +129,50 @@ public abstract class ConfigSubscription<T extends ConfigInstance> { return false; } + void setConfigChanged(boolean changed) { + this.configChanged = changed; + } + + void setGenerationChanged(boolean genChanged) { + this.generationChanged = genChanged; + } /** * Called from {@link ConfigSubscriber} when the changed status of this config is propagated to the clients */ - public boolean isConfigChangedAndReset(Long requiredGen) { - ConfigState<T> prev = config.get(); - while ( prev.getGeneration().equals(requiredGen) && !config.compareAndSet(prev, prev.createUnchanged())) { - prev = config.get(); - } - // A false positive is a lot better than a false negative - return !prev.getGeneration().equals(requiredGen) || prev.isConfigChanged(); + public void resetChangedFlags() { + setConfigChanged(false); + setGenerationChanged(false); } - void setConfig(Long generation, T config) { - this.config.set(new ConfigState<>(true, generation, true, config)); + public boolean isConfigChanged() { + return configChanged; } - // Only used by {@link FileConfigSubscription} - protected void setConfigIncGen(T config) { - ConfigState<T> prev = this.config.get(); - setConfig(prev.getGeneration() + 1, config); + public boolean isGenerationChanged() { + return generationChanged; } - // Only used by {@link FileConfigSubscription} and {@link ConfigSetSubscription} - protected void setConfigIfChangedIncGen(T config) { - ConfigState<T> prev = this.config.get(); - this.config.set(new ConfigState<>(true, prev.getGeneration() + 1, - !config.equals(prev.getConfig()), config)); - } - protected void setConfigIfChanged(T config) { - ConfigState<T> prev = this.config.get(); - this.config.set(new ConfigState<>(true, prev.getGeneration(), - !config.equals(prev.getConfig()), config)); - } - - void setGeneration(Long generation) { - ConfigState<T> c = config.get(); - this.config.set(new ConfigState<>(true, generation, c.isConfigChanged(), c.getConfig())); + void setConfig(T config) { + this.config = config; } /** - * The config state object of this subscription + * The config object of this subscription * * @return the ConfigInstance (the config) of this subscription */ + public T getConfig() { + return config; + } - public ConfigState<T> getConfigState() { - return config.get(); + /** + * The generation of this subscription + * + * @return the generation of this subscription + */ + public Long getGeneration() { + return generation; } /** @@ -212,13 +183,16 @@ public abstract class ConfigSubscription<T extends ConfigInstance> { return configClass; } + void setGeneration(Long generation) { + this.generation = generation; + } + @Override public String toString() { StringBuilder s = new StringBuilder(key.toString()); - ConfigState<T> c = config.get(); - s.append(", Current generation: ").append(c.getGeneration()) - .append(", Generation changed: ").append(c.isGenerationChanged()) - .append(", Config changed: ").append(c.isConfigChanged()); + s.append(", Current generation: ").append(generation) + .append(", Generation changed: ").append(generationChanged) + .append(", Config changed: ").append(configChanged); if (exception != null) s.append(", Exception: ").append(exception); return s.toString(); @@ -238,7 +212,7 @@ public abstract class ConfigSubscription<T extends ConfigInstance> { * that can be set by {@link ConfigSubscriber#reload(long)}. * * @param timeout in milliseconds - * @return false if timed out, true if generation or config or {@link #exception} changed. If true, the {@link #config} field will be set also. + * @return false if timed out, true if the state of {@link #configChanged}, {@link #generationChanged} or {@link #exception} changed. If true, the {@link #config} field will be set also. * has changed */ public abstract boolean nextConfig(long timeout); @@ -303,20 +277,23 @@ public abstract class ConfigSubscription<T extends ConfigInstance> { * Force this into the given generation, used in testing * @param generation a config generation */ - public void reload(long generation) { - reloadedGeneration.set(generation); + public synchronized void reload(long generation) { + this.doReload = true; + this.reloadedGeneration = generation; } /** * True if someone has set the {@link #reloadedGeneration} number by calling {@link #reload(long)} * and hence wants to force a given generation programmatically. If that is the case, - * sets the generation and flags it as changed accordingly. + * sets the {@link #generation} and {@link #generationChanged} fields accordingly. * @return true if {@link #reload(long)} has been called, false otherwise */ - protected boolean checkReloaded() { - Long reloaded = reloadedGeneration.getAndSet(null); - if (reloaded != null) { - setGeneration(reloaded); + protected synchronized boolean checkReloaded() { + if (doReload) { + // User has called reload + generation = reloadedGeneration; + setGenerationChanged(true); + doReload = false; return true; } return false; diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/FileConfigSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/FileConfigSubscription.java index 8c3f87d0702..60c702895f8 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/FileConfigSubscription.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/FileConfigSubscription.java @@ -40,14 +40,15 @@ public class FileConfigSubscription<T extends ConfigInstance> extends ConfigSubs if (checkReloaded()) { log.log(LogLevel.DEBUG, "User forced config reload at " + System.currentTimeMillis()); // User forced reload - setConfigIfChanged(updateConfig()); - ConfigState<T> configState = getConfigState(); - log.log(LogLevel.DEBUG, "Config updated at " + System.currentTimeMillis() + ", changed: " + configState.isConfigChanged()); - log.log(LogLevel.DEBUG, "Config: " + configState.getConfig().toString()); + updateConfig(); + log.log(LogLevel.DEBUG, "Config updated at " + System.currentTimeMillis() + ", changed: " + isConfigChanged()); + log.log(LogLevel.DEBUG, "Config: " + config.toString()); return true; } if (file.lastModified()!=ts) { - setConfigIncGen(updateConfig()); + updateConfig(); + generation++; + setGenerationChanged(true); return true; } try { @@ -55,18 +56,22 @@ public class FileConfigSubscription<T extends ConfigInstance> extends ConfigSubs } catch (InterruptedException e) { throw new ConfigInterruptedException(e); } - + // These shouldn't be checked anywhere since we return false now, but setting them still + setGenerationChanged(false); + setConfigChanged(false); return false; } - private T updateConfig() { + private void updateConfig() { ts=file.lastModified(); + ConfigInstance prev = config; try { ConfigPayload payload = new CfgConfigPayloadBuilder().deserialize(Arrays.asList(IOUtils.readFile(file).split("\n"))); - return payload.toInstance(configClass, key.getConfigId()); + config = payload.toInstance(configClass, key.getConfigId()); } catch (IOException e) { throw new ConfigurationRuntimeException(e); } + setConfigChanged(!config.equals(prev)); } @Override diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/GenericConfigSubscriber.java b/config/src/main/java/com/yahoo/config/subscription/impl/GenericConfigSubscriber.java index a33c1556dd3..7ef94e98cc8 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/GenericConfigSubscriber.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/GenericConfigSubscriber.java @@ -10,7 +10,6 @@ import com.yahoo.config.subscription.ConfigSource; import com.yahoo.config.subscription.ConfigSourceSet; import com.yahoo.config.subscription.ConfigSubscriber; import com.yahoo.vespa.config.ConfigKey; -import com.yahoo.vespa.config.RawConfig; import com.yahoo.vespa.config.TimingValues; /** @@ -44,7 +43,7 @@ public class GenericConfigSubscriber extends ConfigSubscriber { * @param timingValues {@link TimingValues} * @return generic handle */ - public GenericConfigHandle subscribe(ConfigKey<RawConfig> key, List<String> defContent, ConfigSource source, TimingValues timingValues) { + public GenericConfigHandle subscribe(ConfigKey<?> key, List<String> defContent, ConfigSource source, TimingValues timingValues) { checkStateBeforeSubscribe(); GenericJRTConfigSubscription sub = new GenericJRTConfigSubscription(key, defContent, this, source, timingValues); GenericConfigHandle handle = new GenericConfigHandle(sub); 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 068b24cdf1a..ab53ce6dc50 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 @@ -3,6 +3,7 @@ package com.yahoo.config.subscription.impl; import java.util.List; +import com.yahoo.config.ConfigInstance; import com.yahoo.config.subscription.ConfigSource; import com.yahoo.config.subscription.ConfigSubscriber; import com.yahoo.log.LogLevel; @@ -18,22 +19,27 @@ import com.yahoo.vespa.config.protocol.JRTClientConfigRequest; * @author vegardh * */ -public class GenericJRTConfigSubscription extends JRTConfigSubscription<RawConfig> { +@SuppressWarnings("rawtypes") +public class GenericJRTConfigSubscription extends JRTConfigSubscription { + private RawConfig config; private final List<String> defContent; - public GenericJRTConfigSubscription(ConfigKey<RawConfig> key, List<String> defContent, ConfigSubscriber subscriber, - ConfigSource source, TimingValues timingValues) - { + @SuppressWarnings("unchecked") + public GenericJRTConfigSubscription(ConfigKey<?> key, + List<String> defContent, + ConfigSubscriber subscriber, + ConfigSource source, + TimingValues timingValues) { super(key, subscriber, source, timingValues); this.defContent = defContent; } @Override protected void setNewConfig(JRTClientConfigRequest jrtReq) { - setConfig(jrtReq.getNewGeneration(), RawConfig.createFromResponseParameters(jrtReq) ); + this.config = RawConfig.createFromResponseParameters(jrtReq); if (log.isLoggable(LogLevel.DEBUG)) { - log.log(LogLevel.DEBUG, "in setNewConfig, config=" + this.getConfigState().getConfig()); + log.log(LogLevel.DEBUG, "in setNewConfig, config=" + this.config); } } @@ -42,15 +48,13 @@ public class GenericJRTConfigSubscription extends JRTConfigSubscription<RawConfi @Override void setGeneration(Long generation) { super.setGeneration(generation); - ConfigState<RawConfig> configState = getConfigState(); - - if (configState.getConfig() != null) { - configState.getConfig().setGeneration(generation); + if (this.config != null) { + this.config.setGeneration(generation); } } public RawConfig getRawConfig() { - return getConfigState().getConfig(); + return config; } /** @@ -62,4 +66,9 @@ public class GenericJRTConfigSubscription extends JRTConfigSubscription<RawConfi public DefContent getDefContent() { return (DefContent.fromList(defContent)); } + + @Override + public ConfigInstance getConfig() { + return null; + } } diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java index 88af414e28d..7d830484edd 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java @@ -163,7 +163,7 @@ public class JRTConfigRequester implements RequestWaiter { } private void handleFailedRequest(JRTClientConfigRequest jrtReq, JRTConfigSubscription<ConfigInstance> sub, Connection connection) { - final boolean configured = (sub.getConfigState().getConfig() != null); + final boolean configured = (sub.getConfig() != null); if (configured) { // The subscription object has an "old" config, which is all we have to offer back now log.log(LogLevel.INFO, "Failure of config subscription, clients will keep existing config until resolved: " + sub); @@ -226,7 +226,7 @@ public class JRTConfigRequester implements RequestWaiter { if (sub.getState() != ConfigSubscription.State.OPEN) return; fatalFailures++; // The logging depends on whether we are configured or not. - Level logLevel = sub.getConfigState().getConfig() == null ? LogLevel.DEBUG : LogLevel.INFO; + Level logLevel = sub.getConfig() == null ? LogLevel.DEBUG : LogLevel.INFO; String logMessage = "Request for config " + jrtReq.getShortDescription() + "' failed with error code " + jrtReq.errorCode() + " (" + jrtReq.errorMessage() + "), scheduling new connect " + " in " + delay + " ms"; 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 9ae3e226d89..6adeaa80cd1 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 @@ -49,8 +49,7 @@ public class JRTConfigSubscription<T extends ConfigInstance> extends ConfigSubsc public boolean nextConfig(long timeoutMillis) { // These flags may have been left true from a previous call, since ConfigSubscriber's nextConfig // not necessarily returned true and reset the flags then - ConfigState<T> configState = getConfigState(); - boolean gotNew = configState.isGenerationChanged() || configState.isConfigChanged() || hasException(); + boolean gotNew = isGenerationChanged() || isConfigChanged() || hasException(); // Return that now, if there's nothing in queue, so that ConfigSubscriber can move on to other subscriptions to check if (getReqQueue().peek()==null && gotNew) { return true; @@ -61,8 +60,7 @@ public class JRTConfigSubscription<T extends ConfigInstance> extends ConfigSubsc // there is a race here. However: the caller will handle it no matter what it gets from the queue here, // the important part is that local state on the subscription objects is preserved. if (!pollQueue(timeoutMillis)) return gotNew; - configState = getConfigState(); - gotNew = configState.isGenerationChanged() || configState.isConfigChanged() || hasException(); + gotNew = isGenerationChanged() || isConfigChanged() || hasException(); return gotNew; } @@ -85,17 +83,20 @@ public class JRTConfigSubscription<T extends ConfigInstance> extends ConfigSubsc return false; } if (jrtReq.hasUpdatedGeneration()) { + //printStatus(jrtReq, "Updated generation or config"); + setGeneration(jrtReq.getNewGeneration()); + setGenerationChanged(true); if (jrtReq.hasUpdatedConfig()) { + // payload changed setNewConfig(jrtReq); - } else { - setGeneration(jrtReq.getNewGeneration()); + setConfigChanged(true); } } return true; } protected void setNewConfig(JRTClientConfigRequest jrtReq) { - setConfig(jrtReq.getNewGeneration(), toConfigInstance(jrtReq) ); + setConfig(toConfigInstance(jrtReq)); } /** diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/JarConfigSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/JarConfigSubscription.java index 4870d69bb23..2b656b0f498 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/JarConfigSubscription.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/JarConfigSubscription.java @@ -53,7 +53,6 @@ public class JarConfigSubscription<T extends ConfigInstance> extends ConfigSubsc } zipEntry = getEntry(jarFile, path); if (zipEntry==null) throw new IllegalArgumentException("Config '" + key.getName() + "' not found in '" + jarName + "!/" + path + "'."); - T config = null; try { ConfigPayload payload = new CfgConfigPayloadBuilder().deserialize(Arrays.asList(IOUtils.readAll(new InputStreamReader(jarFile.getInputStream(zipEntry), "UTF-8")).split("\n"))); config = payload.toInstance(configClass, key.getConfigId()); @@ -62,7 +61,9 @@ public class JarConfigSubscription<T extends ConfigInstance> extends ConfigSubsc } catch (IOException e) { throw new ConfigurationRuntimeException(e); } - setConfig(0L, config); + setGeneration(0L); + setGenerationChanged(true); + setConfigChanged(true); try { jarFile.close(); } catch (IOException e) { @@ -76,6 +77,9 @@ public class JarConfigSubscription<T extends ConfigInstance> extends ConfigSubsc } catch (InterruptedException e) { throw new ConfigInterruptedException(e); } + // These shouldn't be checked anywhere since we return false now, but setting them still + setGenerationChanged(false); + setConfigChanged(false); return false; } /** diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/RawConfigSubscription.java b/config/src/main/java/com/yahoo/config/subscription/impl/RawConfigSubscription.java index 0d3f04361aa..ce82b62cfd2 100644 --- a/config/src/main/java/com/yahoo/config/subscription/impl/RawConfigSubscription.java +++ b/config/src/main/java/com/yahoo/config/subscription/impl/RawConfigSubscription.java @@ -35,8 +35,12 @@ public class RawConfigSubscription<T extends ConfigInstance> extends ConfigSubsc } if (payload==null) { payload = inputPayload; + setGeneration(0L); + setGenerationChanged(true); + setConfigChanged(true); + ConfigPayload configPayload = new CfgConfigPayloadBuilder().deserialize(Arrays.asList(payload.split("\n"))); - setConfig(0L, configPayload.toInstance(configClass, key.getConfigId())); + config = configPayload.toInstance(configClass, key.getConfigId()); return true; } try { @@ -44,6 +48,9 @@ public class RawConfigSubscription<T extends ConfigInstance> extends ConfigSubsc } catch (InterruptedException e) { throw new ConfigInterruptedException(e); } + // These shouldn't be checked anywhere since we return false now, but setting them still + setGenerationChanged(false); + setConfigChanged(false); return false; } diff --git a/config/src/main/java/com/yahoo/vespa/config/RawConfig.java b/config/src/main/java/com/yahoo/vespa/config/RawConfig.java index 75c5161103e..898e8edfb08 100755 --- a/config/src/main/java/com/yahoo/vespa/config/RawConfig.java +++ b/config/src/main/java/com/yahoo/vespa/config/RawConfig.java @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config; -import com.yahoo.config.ConfigInstance; import com.yahoo.text.Utf8String; import com.yahoo.vespa.config.protocol.CompressionInfo; import com.yahoo.vespa.config.protocol.JRTClientConfigRequest; @@ -21,7 +20,7 @@ import java.util.Optional; * * @author hmusum */ -public class RawConfig extends ConfigInstance { +public class RawConfig { private final ConfigKey<?> key; private final String defMd5; diff --git a/config/src/main/java/com/yahoo/vespa/config/protocol/JRTClientConfigRequestV3.java b/config/src/main/java/com/yahoo/vespa/config/protocol/JRTClientConfigRequestV3.java index 3d7c0dc1e80..4ba6a74ce6f 100644 --- a/config/src/main/java/com/yahoo/vespa/config/protocol/JRTClientConfigRequestV3.java +++ b/config/src/main/java/com/yahoo/vespa/config/protocol/JRTClientConfigRequestV3.java @@ -2,7 +2,6 @@ package com.yahoo.vespa.config.protocol; import com.yahoo.config.ConfigInstance; -import com.yahoo.config.subscription.impl.ConfigSubscription; import com.yahoo.config.subscription.impl.JRTConfigSubscription; import com.yahoo.jrt.Request; import com.yahoo.text.Utf8Array; @@ -74,13 +73,12 @@ public class JRTClientConfigRequestV3 extends SlimeClientConfigRequest { public static <T extends ConfigInstance> JRTClientConfigRequest createFromSub(JRTConfigSubscription<T> sub, Trace trace, CompressionType compressionType, Optional<VespaVersion> vespaVersion) { String hostname = ConfigUtils.getCanonicalHostName(); ConfigKey<T> key = sub.getKey(); - ConfigSubscription.ConfigState<T> configState = sub.getConfigState(); - T i = configState.getConfig(); + T i = sub.getConfig(); return createWithParams(key, sub.getDefContent(), hostname, i != null ? i.getConfigMd5() : "", - configState.getGeneration() != null ? configState.getGeneration() : 0L, + sub.getGeneration() != null ? sub.getGeneration() : 0L, sub.timingValues().getSubscribeTimeout(), trace, compressionType, diff --git a/config/src/test/java/com/yahoo/config/subscription/impl/FileConfigSubscriptionTest.java b/config/src/test/java/com/yahoo/config/subscription/impl/FileConfigSubscriptionTest.java index 8fdd9d5e3b3..3862cfc9069 100644 --- a/config/src/test/java/com/yahoo/config/subscription/impl/FileConfigSubscriptionTest.java +++ b/config/src/test/java/com/yahoo/config/subscription/impl/FileConfigSubscriptionTest.java @@ -18,8 +18,6 @@ import java.nio.file.Files; import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotSame; -import static org.junit.Assert.assertSame; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -50,11 +48,11 @@ public class FileConfigSubscriptionTest { subscriber, TEST_TYPES_FILE); assertTrue(sub.nextConfig(1000)); - assertThat(sub.getConfigState().getConfig().intval(), is(23)); + assertThat(sub.config.intval(), is(23)); Thread.sleep(1000); writeConfig("intval", "33"); assertTrue(sub.nextConfig(1000)); - assertThat(sub.getConfigState().getConfig().intval(), is(33)); + assertThat(sub.config.intval(), is(33)); } @Test @@ -66,37 +64,18 @@ public class FileConfigSubscriptionTest { subscriber, TEST_TYPES_FILE); assertTrue(sub.nextConfig(1000)); - assertThat(sub.getConfigState().getConfig().intval(), is(23)); + assertThat(sub.config.intval(), is(23)); writeConfig("intval", "33"); sub.reload(1); assertTrue(sub.nextConfig(1000)); - ConfigSubscription.ConfigState<SimpletypesConfig> configState = sub.getConfigState(); - assertThat(configState.getConfig().intval(), is(33)); - assertTrue(configState.isConfigChanged()); - assertTrue(configState.isGenerationChanged()); - - assertTrue(sub.isConfigChangedAndReset(7L)); - assertSame(configState, sub.getConfigState()); - assertTrue(configState.isConfigChanged()); - assertTrue(configState.isGenerationChanged()); - assertTrue(sub.isConfigChangedAndReset(1L)); - assertNotSame(configState, sub.getConfigState()); - configState = sub.getConfigState(); - assertFalse(configState.isConfigChanged()); - assertFalse(configState.isGenerationChanged()); - + assertThat(sub.config.intval(), is(33)); + assertTrue(sub.isConfigChanged()); + assertTrue(sub.isGenerationChanged()); sub.reload(2); assertTrue(sub.nextConfig(1000)); - configState = sub.getConfigState(); - assertThat(configState.getConfig().intval(), is(33)); - assertFalse(configState.isConfigChanged()); - assertTrue(configState.isGenerationChanged()); - - assertFalse(sub.isConfigChangedAndReset(2L)); - assertNotSame(configState, sub.getConfigState()); - configState = sub.getConfigState(); - assertFalse(configState.isConfigChanged()); - assertFalse(configState.isGenerationChanged()); + assertThat(sub.config.intval(), is(33)); + assertFalse(sub.isConfigChanged()); + assertTrue(sub.isGenerationChanged()); } @Test @@ -107,7 +86,7 @@ public class FileConfigSubscriptionTest { ConfigSubscriber subscriber = new ConfigSubscriber(); ConfigSubscription<TestReferenceConfig> sub = ConfigSubscription.get(key, subscriber, new DirSource(new File(cfgDir)), new TimingValues()); assertTrue(sub.nextConfig(1000)); - assertThat(sub.getConfigState().getConfig().configId(), is(cfgId)); + assertThat(sub.config.configId(), is(cfgId)); } @Test(expected = IllegalArgumentException.class) diff --git a/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java b/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java index 1ad66cbaf58..402c3b9b23f 100644 --- a/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java +++ b/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java @@ -181,7 +181,8 @@ public class JRTConfigRequesterTest { ConfigSubscriber subscriber = new ConfigSubscriber(); final TimingValues timingValues = getTestTimingValues(); JRTConfigSubscription<SimpletypesConfig> sub = createSubscription(subscriber, timingValues); - sub.setConfig(1L, config()); + sub.setConfig(config()); + sub.setGeneration(1L); final MockConnection connection = new MockConnection(new ErrorResponseHandler()); JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues); @@ -209,7 +210,8 @@ public class JRTConfigRequesterTest { ConfigSubscriber subscriber = new ConfigSubscriber(); final TimingValues timingValues = getTestTimingValues(); JRTConfigSubscription<SimpletypesConfig> sub = createSubscription(subscriber, timingValues); - sub.setConfig(1L, config()); + sub.setConfig(config()); + sub.setGeneration(1L); final MockConnection connection = new MockConnection(new ErrorResponseHandler(com.yahoo.jrt.ErrorCode.TIMEOUT)); JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues); @@ -224,7 +226,8 @@ public class JRTConfigRequesterTest { ConfigSubscriber subscriber = new ConfigSubscriber(); final TimingValues timingValues = getTestTimingValues(); JRTConfigSubscription<SimpletypesConfig> sub = createSubscription(subscriber, timingValues); - sub.setConfig(1L, config()); + sub.setConfig(config()); + sub.setGeneration(1L); final MockConnection connection = new MockConnection(new ErrorResponseHandler(ErrorCode.UNKNOWN_DEFINITION)); JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues); diff --git a/config/src/test/java/com/yahoo/vespa/config/protocol/JRTConfigRequestBase.java b/config/src/test/java/com/yahoo/vespa/config/protocol/JRTConfigRequestBase.java index 1f5274b832b..7fa205fef15 100644 --- a/config/src/test/java/com/yahoo/vespa/config/protocol/JRTConfigRequestBase.java +++ b/config/src/test/java/com/yahoo/vespa/config/protocol/JRTConfigRequestBase.java @@ -249,7 +249,7 @@ public abstract class JRTConfigRequestBase { assertTrue(sub.nextConfig(120_0000)); sub.close(); JRTClientConfigRequest nextReq = createReq(sub, Trace.createNew()); - SimpletypesConfig config = sub.getConfigState().getConfig(); + SimpletypesConfig config = sub.getConfig(); assertThat(nextReq.getRequestConfigMd5(), is(config.getConfigMd5())); assertThat(nextReq.getRequestGeneration(), is(currentGeneration)); System.setProperty("VESPA_CONFIG_PROTOCOL_VERSION", ""); |