diff options
author | Harald Musum <musum@yahooinc.com> | 2022-04-18 14:25:18 +0200 |
---|---|---|
committer | Harald Musum <musum@yahooinc.com> | 2022-04-18 14:25:18 +0200 |
commit | f9c03010a3765436f1e384c2b04a7c4dfb3d5b26 (patch) | |
tree | 5989ff67462daeddfe98b6625bcf34c235a2db90 /config-proxy | |
parent | 1d08ac1b7fafcbd84e32afa7e63487e52a6a24f9 (diff) |
Use Optional for getConfig/resolveConfig calls
Some minor changes to log output as well
Diffstat (limited to 'config-proxy')
11 files changed, 72 insertions, 76 deletions
diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigProxyRpcServer.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigProxyRpcServer.java index 158df654439..7b8deb19831 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigProxyRpcServer.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigProxyRpcServer.java @@ -19,12 +19,15 @@ import com.yahoo.vespa.config.protocol.JRTServerConfigRequestV3; import java.util.Arrays; import java.util.Iterator; +import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.logging.Level; import java.util.logging.Logger; +import static com.yahoo.vespa.config.ErrorCode.INTERNAL_ERROR; + /** * An RPC server that handles config and file distribution requests. * @@ -247,25 +250,26 @@ public class ConfigProxyRpcServer implements Runnable, TargetWatcher { private void getConfigImpl(JRTServerConfigRequest request) { ResponseHandler responseHandler = new ResponseHandler(); request.getRequestTrace().trace(TRACELEVEL, "Config proxy getConfig()"); - log.log(Level.FINE, () ->"getConfig: " + request.getShortDescription() + ",config checksums=" + request.getRequestConfigChecksums()); + log.log(Level.FINE, () ->"getConfig: " + request); if (!request.validateParameters()) { - // Error code is set in verifyParameters if parameters are not OK. - log.log(Level.WARNING, "Parameters for request " + request + " did not validate: " + request.errorCode() + " : " + request.errorMessage()); - responseHandler.returnErrorResponse(request, request.errorCode(), "Parameters for request " + request.getShortDescription() + " did not validate: " + request.errorMessage()); + // Error code is set in validateParameters if parameters are not OK. + log.log(Level.WARNING, "Invalid parameters for request " + request + ": " + request.errorCode() + " : " + request.errorMessage()); + responseHandler.returnErrorResponse(request, request.errorCode(), "Invalid parameters for request " + request + + ": " + request.errorMessage()); return; } + try { - RawConfig config = proxyServer.resolveConfig(request); - if (config == null) { - log.log(Level.FINEST, () -> "No config received yet for " + request.getShortDescription() + ", not sending response"); - } else if (ProxyServer.configOrGenerationHasChanged(config, request)) { - responseHandler.returnOkResponse(request, config); - } else { - log.log(Level.FINEST, () -> "No new config for " + request.getShortDescription() + ", not sending response"); - } + Optional<RawConfig> config = proxyServer.resolveConfig(request); + if (config.isEmpty()) + log.log(Level.FINEST, () -> "No config received yet for " + request + ", not sending response"); + else if (ProxyServer.configOrGenerationHasChanged(config.get(), request)) + responseHandler.returnOkResponse(request, config.get()); + else + log.log(Level.FINEST, () -> "No new config for " + request + ", not sending response"); } catch (Exception e) { - e.printStackTrace(); - responseHandler.returnErrorResponse(request, com.yahoo.vespa.config.ErrorCode.INTERNAL_ERROR, e.getMessage()); + log.log(Level.WARNING, "Resolving config " + request + " failed", e); + responseHandler.returnErrorResponse(request, INTERNAL_ERROR, e.getMessage()); } } diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java index dae732e56ec..d771d440078 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/ConfigSourceClient.java @@ -5,6 +5,7 @@ import com.yahoo.vespa.config.RawConfig; import com.yahoo.vespa.config.protocol.JRTServerConfigRequest; import java.util.List; +import java.util.Optional; /** * A client to a config source, which could be an RPC config server or some other backing for @@ -14,7 +15,7 @@ import java.util.List; */ interface ConfigSourceClient { - RawConfig getConfig(RawConfig input, JRTServerConfigRequest request); + Optional<RawConfig> getConfig(RawConfig input, JRTServerConfigRequest request); void shutdown(); diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/DelayedResponseHandler.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/DelayedResponseHandler.java index 0e8ebe0d9c9..cae70b41c8c 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/DelayedResponseHandler.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/DelayedResponseHandler.java @@ -6,6 +6,7 @@ import com.yahoo.vespa.config.RawConfig; import com.yahoo.vespa.config.protocol.JRTServerConfigRequest; import com.yahoo.yolean.Exceptions; +import java.util.Optional; import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; import java.util.logging.Logger; @@ -47,9 +48,9 @@ public class DelayedResponseHandler implements Runnable { while ((response = delayedResponses.responses().poll()) != null) { JRTServerConfigRequest request = response.getRequest(); ConfigCacheKey cacheKey = new ConfigCacheKey(request.getConfigKey(), request.getRequestDefMd5()); - RawConfig config = memoryCache.get(cacheKey); - if (config != null) { - responseHandler.returnOkResponse(request, config); + Optional<RawConfig> config = memoryCache.get(cacheKey); + if (config.isPresent()) { + responseHandler.returnOkResponse(request, config.get()); sentResponses.incrementAndGet(); } else { log.log(Level.WARNING, "Timed out (timeout " + request.getTimeout() + ") getting config " + diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCache.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCache.java index e2ab5de40e9..ff10adf88e9 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCache.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCache.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.io.Writer; import java.nio.file.Files; import java.util.Collection; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; @@ -31,8 +32,8 @@ public class MemoryCache { private final ConcurrentHashMap<ConfigCacheKey, RawConfig> cache = new ConcurrentHashMap<>(500, 0.75f); - public RawConfig get(ConfigCacheKey key) { - return cache.get(key); + public Optional<RawConfig> get(ConfigCacheKey key) { + return Optional.ofNullable(cache.get(key)); } /** diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java index f1be03f07d4..d207fcce639 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClient.java @@ -8,6 +8,7 @@ import com.yahoo.vespa.config.protocol.JRTServerConfigRequest; import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.logging.Level; import java.util.logging.Logger; @@ -33,16 +34,13 @@ class MemoryCacheConfigClient implements ConfigSourceClient { * @return A Config with a payload. */ @Override - public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) { - log.log(Level.FINE, () -> "Getting config from cache"); + public Optional<RawConfig> getConfig(RawConfig input, JRTServerConfigRequest request) { ConfigKey<?> key = input.getKey(); - RawConfig cached = cache.get(new ConfigCacheKey(key, input.getDefMd5())); - if (cached != null) { + + Optional<RawConfig> cached = cache.get(new ConfigCacheKey(key, input.getDefMd5())); + if (cached.isPresent()) log.log(Level.FINE, () -> "Found config " + key + " in cache"); - return cached; - } else { - return null; - } + return cached; } @Override 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 9340363def8..a559f3025ed 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 @@ -15,6 +15,7 @@ import com.yahoo.yolean.system.CatchSignals; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -71,11 +72,11 @@ public class ProxyServer implements Runnable { } } - RawConfig resolveConfig(JRTServerConfigRequest req) { + Optional<RawConfig> resolveConfig(JRTServerConfigRequest req) { // Calling getConfig() will either return with an answer immediately or // 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. + // calls updateSubscribers when new config is returned from the config server. + // In the last case the method below will return empty. return configClient.getConfig(RawConfig.createFromServerRequest(req), req); } 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 f022ed11f3d..8f844fdd3ee 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 @@ -110,7 +110,7 @@ class RpcConfigSourceClient implements ConfigSourceClient, Runnable { * @return A Config with a payload. */ @Override - public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) { + public Optional<RawConfig> getConfig(RawConfig input, JRTServerConfigRequest request) { // 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 @@ -119,29 +119,29 @@ class RpcConfigSourceClient implements ConfigSourceClient, Runnable { delayedResponses.add(delayedResponse); ConfigCacheKey configCacheKey = new ConfigCacheKey(input.getKey(), input.getDefMd5()); - RawConfig cachedConfig = memoryCache.get(configCacheKey); + Optional<RawConfig> cachedConfig = memoryCache.get(configCacheKey); boolean needToGetConfig = true; - RawConfig ret = null; - if (cachedConfig != null) { - log.log(Level.FINE, () -> "Found config " + configCacheKey + " in cache, generation=" + cachedConfig.getGeneration() + - ",config checksums=" + cachedConfig.getPayloadChecksums()); - log.log(Level.FINEST, () -> "input config=" + input + ",cached config=" + cachedConfig); - if (ProxyServer.configOrGenerationHasChanged(cachedConfig, request)) { + if (cachedConfig.isPresent()) { + RawConfig config = cachedConfig.get(); + log.log(Level.FINE, () -> "Found config " + configCacheKey + " in cache, generation=" + config.getGeneration() + + ",config checksums=" + config.getPayloadChecksums()); + log.log(Level.FINEST, () -> "input config=" + input + ",cached config=" + config); + if (ProxyServer.configOrGenerationHasChanged(config, request)) { log.log(Level.FINEST, () -> "Cached config is not equal to requested, will return it"); if (delayedResponses.remove(delayedResponse)) { // unless another thread already did it - ret = cachedConfig; + return cachedConfig; } } - if (!cachedConfig.isError() && cachedConfig.getGeneration() > 0) { + if (!config.isError() && config.getGeneration() > 0) { needToGetConfig = false; } } if (needToGetConfig) { subscribeToConfig(input, configCacheKey); } - return ret; + return Optional.empty(); } private void subscribeToConfig(RawConfig input, ConfigCacheKey configCacheKey) { diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClientTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClientTest.java index f84d447e3a1..d243d9c6dff 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClientTest.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheConfigClientTest.java @@ -6,7 +6,7 @@ import org.junit.Test; import java.util.List; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; /** * @author hmusum @@ -17,8 +17,8 @@ public class MemoryCacheConfigClientTest { public void basic() { MemoryCacheConfigClient client = new MemoryCacheConfigClient(new MemoryCache()); client.memoryCache().update(ConfigTester.fooConfig); - assertEquals(ConfigTester.fooConfig, client.getConfig(ConfigTester.fooConfig, null)); - assertNull(client.getConfig(ConfigTester.barConfig, null)); + assertEquals(ConfigTester.fooConfig, client.getConfig(ConfigTester.fooConfig, null).orElseThrow()); + assertTrue(client.getConfig(ConfigTester.barConfig, null).isEmpty()); assertEquals("N/A", client.getActiveSourceConnection()); assertEquals(List.of("N/A"), client.getSourceConnections()); diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheTest.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheTest.java index 1a919ad3988..b0cba728a0c 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheTest.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MemoryCacheTest.java @@ -15,7 +15,6 @@ import java.util.ArrayList; import java.util.Optional; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; /** @@ -84,14 +83,12 @@ public class MemoryCacheTest { assertTrue(cache.containsKey(cacheKey)); assertTrue(cache.containsKey(cacheKey2)); - RawConfig response = cache.get(cacheKey); - assertNotNull(response); + RawConfig response = cache.get(cacheKey).orElseThrow(); assertEquals(defName, response.getName()); assertEquals(payload.toString(), response.getPayload().toString()); assertEquals(generation, response.getGeneration()); - response = cache.get(cacheKey2); - assertNotNull(response); + response = cache.get(cacheKey2).orElseThrow(); assertEquals(defName2, response.getName()); assertEquals(payload2.toString(), response.getPayload().toString()); assertEquals(generation, response.getGeneration()); @@ -108,14 +105,12 @@ public class MemoryCacheTest { assertEquals(2, cache.size()); assertTrue(cache.containsKey(cacheKey)); - RawConfig response = cache.get(cacheKey); - assertNotNull(response); + RawConfig response = cache.get(cacheKey).orElseThrow(); assertEquals(defName, response.getName()); assertEquals(payload.getData(), response.getPayload().getData()); assertEquals(generation, response.getGeneration()); - response = cache.get(cacheKeyDifferentMd5); - assertNotNull(response); + response = cache.get(cacheKeyDifferentMd5).orElseThrow(); assertEquals(defName, response.getName()); assertEquals(payloadDifferentMd5.getData(), response.getPayload().getData()); assertEquals(generation, response.getGeneration()); diff --git a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MockConfigSourceClient.java b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MockConfigSourceClient.java index d0724b9dbd0..e02ca22d0a0 100644 --- a/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MockConfigSourceClient.java +++ b/config-proxy/src/test/java/com/yahoo/vespa/config/proxy/MockConfigSourceClient.java @@ -7,6 +7,7 @@ import com.yahoo.vespa.config.protocol.JRTServerConfigRequest; import java.util.Collections; import java.util.List; +import java.util.Optional; /** * Mock client that always returns with config immediately @@ -24,10 +25,10 @@ public class MockConfigSourceClient implements ConfigSourceClient{ } @Override - public RawConfig getConfig(RawConfig input, JRTServerConfigRequest request) { - final RawConfig config = getConfig(input.getKey()); + public Optional<RawConfig> getConfig(RawConfig input, JRTServerConfigRequest request) { + RawConfig config = getConfig(input.getKey()); memoryCache.update(config); - return config; + return Optional.of(config); } private RawConfig getConfig(ConfigKey<?> configKey) { 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 09eae6a297d..1bcd1562a85 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 @@ -62,11 +62,10 @@ public class ProxyServerTest { ConfigTester tester = new ConfigTester(); MemoryCache memoryCache = proxy.memoryCache(); assertEquals(0, memoryCache.size()); - RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)); - assertNotNull(res); + RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertEquals(ConfigTester.fooPayload.toString(), res.getPayload().toString()); assertEquals(1, memoryCache.size()); - assertEquals(res, memoryCache.get(new ConfigCacheKey(fooConfig.getKey(), fooConfig.getDefMd5()))); + assertEquals(res, memoryCache.get(new ConfigCacheKey(fooConfig.getKey(), fooConfig.getDefMd5())).orElseThrow()); } /** @@ -111,15 +110,14 @@ public class ProxyServerTest { ConfigTester tester = new ConfigTester(); MemoryCache memoryCache = proxy.memoryCache(); assertEquals(0, memoryCache.size()); - RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)); - assertNotNull(res); + RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertEquals(ConfigTester.fooPayload.toString(), res.getPayload().toString()); assertEquals(1, memoryCache.size()); - assertEquals(res, memoryCache.get(new ConfigCacheKey(fooConfig.getKey(), fooConfig.getDefMd5()))); + assertEquals(res, memoryCache.get(new ConfigCacheKey(fooConfig.getKey(), fooConfig.getDefMd5())).orElseThrow()); // Trying same config again JRTServerConfigRequest newRequestBasedOnResponse = tester.createRequest(res); - RawConfig res2 = proxy.resolveConfig(newRequestBasedOnResponse); + RawConfig res2 = proxy.resolveConfig(newRequestBasedOnResponse).orElseThrow(); assertFalse(ProxyServer.configOrGenerationHasChanged(res2, newRequestBasedOnResponse)); assertEquals(1, memoryCache.size()); } @@ -137,8 +135,7 @@ public class ProxyServerTest { MemoryCache memoryCache = proxy.memoryCache(); assertEquals(0, memoryCache.size()); - RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)); - assertNotNull(res); + RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertNotNull(res.getPayload()); assertTrue(res.isError()); assertEquals(0, memoryCache.size()); @@ -148,14 +145,13 @@ public class ProxyServerTest { source.put(fooConfig.getKey(), createConfigWithNextConfigGeneration(fooConfig, 0)); // Verify that we get the config now and that it is cached - res = proxy.resolveConfig(tester.createRequest(fooConfig)); - assertNotNull(res); + res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertNotNull(res.getPayload().getData()); assertEquals(ConfigTester.fooPayload.toString(), res.getPayload().toString()); assertEquals(1, memoryCache.size()); JRTServerConfigRequest newRequestBasedOnResponse = tester.createRequest(res); - RawConfig res2 = proxy.resolveConfig(newRequestBasedOnResponse); + RawConfig res2 = proxy.resolveConfig(newRequestBasedOnResponse).orElseThrow(); assertFalse(ProxyServer.configOrGenerationHasChanged(res2, newRequestBasedOnResponse)); assertEquals(1, memoryCache.size()); } @@ -172,7 +168,7 @@ public class ProxyServerTest { MemoryCache cache = proxy.memoryCache(); assertEquals(0, cache.size()); - RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)); + RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertNotNull(res); assertEquals(ConfigTester.fooPayload.toString(), res.getPayload().toString()); assertEquals(1, cache.size()); @@ -183,8 +179,7 @@ public class ProxyServerTest { 0, fooConfig.getDefContent(), Optional.empty()); source.put(fooConfig.getKey(), emptyConfig); - res = proxy.resolveConfig(tester.createRequest(fooConfig)); - assertNotNull(res.getPayload()); + res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertEquals(emptyConfig.getPayload().toString(), res.getPayload().toString()); assertEquals(0, cache.size()); @@ -193,7 +188,7 @@ public class ProxyServerTest { source.put(fooConfig.getKey(), createConfigWithNextConfigGeneration(fooConfig, 0)); // Verify that we get the config now and that it is cached - res = proxy.resolveConfig(tester.createRequest(fooConfig)); + res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertNotNull(res.getPayload().getData()); assertEquals(ConfigTester.fooPayload.toString(), res.getPayload().toString()); assertEquals(1, cache.size()); @@ -202,15 +197,14 @@ public class ProxyServerTest { @Test public void testReconfiguration() { ConfigTester tester = new ConfigTester(); - RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)); - assertNotNull(res); + RawConfig res = proxy.resolveConfig(tester.createRequest(fooConfig)).orElseThrow(); assertEquals(ConfigTester.fooPayload.toString(), res.getPayload().toString()); // Simulate deployment, add config with new config generation long previousGeneration = res.getGeneration(); source.put(fooConfig.getKey(), createConfigWithNextConfigGeneration(res, 0)); JRTServerConfigRequest newRequestBasedOnResponse = tester.createRequest(res); - RawConfig res2 = proxy.resolveConfig(newRequestBasedOnResponse); + RawConfig res2 = proxy.resolveConfig(newRequestBasedOnResponse).orElseThrow(); assertEquals(previousGeneration + 1, res2.getGeneration()); assertTrue(ProxyServer.configOrGenerationHasChanged(res2, newRequestBasedOnResponse)); } |