diff options
author | Harald Musum <musum@yahoo-inc.com> | 2017-03-08 08:15:22 +0100 |
---|---|---|
committer | Harald Musum <musum@yahoo-inc.com> | 2017-03-08 08:15:22 +0100 |
commit | 970218183ff6bf1b90c56fc00b9482e3b5f64348 (patch) | |
tree | dc59a5181b464d06142458d89608fbc1a3ced4c2 /config-proxy | |
parent | 12d429744ccdf13c2ff5aa6d343b7b823d93b33d (diff) |
Avoid race when getting config
* Always add to delayed responses (we remove instead if we find config in cache)
This is to avoid a race where we might end up not adding to delayed responses
nor subscribing to config if another request for the same config
happens at the same time
* Add to cache only in one place
* Remove meaningless test
Diffstat (limited to 'config-proxy')
8 files changed, 42 insertions, 51 deletions
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java index 28fc22538b6..f4ae5e130e1 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ClientUpdater.java @@ -104,4 +104,9 @@ class ClientUpdater { log.log(LogLevel.DEBUG, "Finished updating config for " + config.getKey() + "," + config.getGeneration()); } } + + // TODO: Remove, temporary until MapBackedConfigSource has been refactored + public MemoryCache getMemoryCache() { + return memoryCache; + } } diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java index e8a76257b14..57adaa572bd 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MapBackedConfigSource.java @@ -17,6 +17,7 @@ import java.util.List; * @author hmusum * @since 5.1.10 */ +// TODO Move to src/test/ public class MapBackedConfigSource implements ConfigSource, ConfigSourceClient { private final HashMap<ConfigKey<?>, RawConfig> backing = new HashMap<>(); private final ClientUpdater clientUpdater; @@ -33,7 +34,9 @@ public class MapBackedConfigSource implements ConfigSource, ConfigSourceClient { @Override public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) { - return getConfig(input.getKey()); + final RawConfig config = getConfig(input.getKey()); + clientUpdater.getMemoryCache().put(config); + return config; } RawConfig getConfig(ConfigKey<?> configKey) { diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java index c5df12f1e96..463fa163b20 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ProxyServer.java @@ -85,19 +85,21 @@ public class ProxyServer implements Runnable { } static ProxyServer createTestServer(ConfigSourceSet source) { - return createTestServer(source, null); + return createTestServer(source, null, new MemoryCache()); } - static ProxyServer createTestServer(MapBackedConfigSource source) { - return createTestServer(source, source); + static ProxyServer createTestServer(MapBackedConfigSource source, MemoryCache memoryCache) { + return createTestServer(source, source, memoryCache); } - private static ProxyServer createTestServer(ConfigSource source, ConfigSourceClient configSourceClient) { + private static ProxyServer createTestServer(ConfigSource source, + ConfigSourceClient configSourceClient, + MemoryCache memoryCache) { final ConfigProxyStatistics statistics = new ConfigProxyStatistics(); final boolean delayedResponseHandling = false; return new ProxyServer(null, new DelayedResponses(statistics), source, statistics, defaultTimingValues(), delayedResponseHandling, - new MemoryCache(), configSourceClient); + memoryCache, configSourceClient); } public void run() { @@ -123,11 +125,7 @@ public class ProxyServer implements Runnable { // create a background thread that retrieves config from the server and // calls updateSubscribers when new config is returned from the config source. // In the last case the method below will return null. - RawConfig config = configClient.getConfig(RawConfig.createFromServerRequest(req), req); - if (configOrGenerationHasChanged(config, req)) { - memoryCache.put(config); - } - return config; + return configClient.getConfig(RawConfig.createFromServerRequest(req), req); } static boolean configOrGenerationHasChanged(RawConfig config, JRTServerConfigRequest request) { diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java index b0ad57e8a6c..a2df139ad5d 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/RpcConfigSourceClient.java @@ -112,34 +112,37 @@ class RpcConfigSourceClient implements ConfigSourceClient { */ @Override public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) { - long start = System.currentTimeMillis(); - RawConfig ret = null; + // Always add to delayed responses (we remove instead if we find config in cache) + // This is to avoid a race where we might end up not adding to delayed responses + // nor subscribing to config if another request for the same config + // happens at the same time + DelayedResponse delayedResponse = new DelayedResponse(request); + delayedResponses.add(delayedResponse); + final ConfigCacheKey configCacheKey = new ConfigCacheKey(input.getKey(), input.getDefMd5()); RawConfig cachedConfig = memoryCache.get(configCacheKey); boolean needToGetConfig = true; + RawConfig ret = null; if (cachedConfig != null) { log.log(LogLevel.DEBUG, "Found config " + configCacheKey + " in cache, generation=" + cachedConfig.getGeneration() + ",configmd5=" + cachedConfig.getConfigMd5()); if (log.isLoggable(LogLevel.SPAM)) { log.log(LogLevel.SPAM, "input config=" + input + ",cached config=" + cachedConfig); } - // equals() also takes generation into account - if (!cachedConfig.equals(input)) { + if (ProxyServer.configOrGenerationHasChanged(cachedConfig, request)) { log.log(LogLevel.SPAM, "Cached config is not equal to requested, will return it"); ret = cachedConfig; + + // Someone else has replied + if (! delayedResponses.remove(delayedResponse)) { + ret = null; + } } if (!cachedConfig.isError()) { needToGetConfig = false; } } - if (!ProxyServer.configOrGenerationHasChanged(ret, request)) { - if (log.isLoggable(LogLevel.DEBUG)) { - log.log(LogLevel.DEBUG, "Delaying response " + request.getShortDescription() + " (" + - (System.currentTimeMillis() - start) + " ms)"); - } - delayedResponses.add(new DelayedResponse(request)); - } if (needToGetConfig) { subscribeToConfig(input, configCacheKey); } 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 7a8a4436749..85e60e28b70 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 @@ -68,6 +68,7 @@ public class UpstreamConfigSubscriber implements Subscriber { "', generation=" + newConfig.getGeneration() + ", payload=" + newConfig.getPayload()); } + // memoryCache.put(); TODO: Add here later and remove in ClientUpdater clientUpdater.updateSubscribers(newConfig); } diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java index f063427d808..7ba4d676000 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/DelayedResponseHandlerTest.java @@ -14,7 +14,7 @@ import static org.junit.Assert.assertThat; */ public class DelayedResponseHandlerTest { - private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create()); + private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create(new MemoryCache())); @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java index ed6d2056733..47b4dd4d1ea 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/ProxyServerTest.java @@ -21,8 +21,9 @@ import static org.junit.Assert.*; */ public class ProxyServerTest { - private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create()); - private ProxyServer proxy = ProxyServer.createTestServer(source); + MemoryCache memoryCache = new MemoryCache(); + private final MapBackedConfigSource source = new MapBackedConfigSource(UpstreamConfigSubscriberTest.MockClientUpdater.create(memoryCache)); + private ProxyServer proxy = ProxyServer.createTestServer(source, memoryCache); static final RawConfig fooConfig = Helper.fooConfigV2; @@ -40,7 +41,7 @@ public class ProxyServerTest { source.clear(); source.put(fooConfig.getKey(), createConfigWithNextConfigGeneration(fooConfig, 0)); source.put(errorConfigKey, createConfigWithNextConfigGeneration(fooConfig, ErrorCode.UNKNOWN_DEFINITION)); - proxy = ProxyServer.createTestServer(source); + proxy = ProxyServer.createTestServer(source, memoryCache); } @After @@ -81,26 +82,6 @@ public class ProxyServerTest { } /** - * Tests that the proxy server can be tested with a MapBackedConfigSource, - * which is a simple hash map with configs - */ - @Test - public void testRawConfigSetBasics() { - ConfigTester tester = new ConfigTester(); - JRTServerConfigRequest errorConfigRequest = tester.createRequest(errorConfig); - - assertTrue(proxy.getMode().isDefault()); - RawConfig config = proxy.resolveConfig(Helper.fooConfigRequest); - assertThat(config, is(createConfigWithNextConfigGeneration(Helper.fooConfig, 0))); - - config = proxy.resolveConfig(Helper.barConfigRequest); - assertNull(config); - - config = proxy.resolveConfig(errorConfigRequest); - assertThat(config.errorCode(), is(ErrorCode.UNKNOWN_DEFINITION)); - } - - /** * Verifies that config is retrieved from the real server when it is not found in the cache, * that the cache is populated with the config and that the entry in the cache is used * when it is found there. diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java index 27ee550d956..e566df8bb2a 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/UpstreamConfigSubscriberTest.java @@ -43,7 +43,7 @@ public class UpstreamConfigSubscriberTest { @Before public void setup() { - clientUpdater = MockClientUpdater.create(); + clientUpdater = MockClientUpdater.create(new MemoryCache()); sourceResponses = new MapBackedConfigSource(clientUpdater); ConfigPayload payload = getConfigPayload("bar", "value"); @@ -147,14 +147,14 @@ public class UpstreamConfigSubscriberTest { static class MockClientUpdater extends ClientUpdater { private RawConfig lastConfig; - private MockClientUpdater(ConfigProxyStatistics statistics, Mode mode) { - super(new MemoryCache(), new MockRpcServer(), statistics, new DelayedResponses(statistics), mode); + private MockClientUpdater(ConfigProxyStatistics statistics, Mode mode, MemoryCache memoryCache) { + super(memoryCache, new MockRpcServer(), statistics, new DelayedResponses(statistics), mode); } - public static MockClientUpdater create() { + static MockClientUpdater create(MemoryCache memoryCache) { Mode mode = new Mode(); ConfigProxyStatistics statistics = new ConfigProxyStatistics(); - return new MockClientUpdater(statistics, mode); + return new MockClientUpdater(statistics, mode, memoryCache); } @Override |