From 674812b71864455c3b81c12428ea4830c3bce8a0 Mon Sep 17 00:00:00 2001 From: gautamomento <89037104+gautamomento@users.noreply.github.com> Date: Wed, 29 Sep 2021 15:55:06 -0700 Subject: [PATCH] feat: Cache Exception handling and CI Updates (#45) * feat: Exception handling and integration test for Cache Also attempting to enable ci * Add ci * formatting fixes * fix tracing test * formatting fixes * formatting --- .github/workflows/ci.yml | 4 +- .../intTest/java/momento/sdk/CacheTest.java | 167 +++++++++--------- .../intTest/java/momento/sdk/MomentoTest.java | 65 ++++--- .../intTest/java/momento/sdk/TestHelpers.java | 11 ++ .../src/main/java/momento/sdk/Cache.java | 33 ++-- .../src/main/java/momento/sdk/Momento.java | 2 +- .../exceptions/CacheNotFoundException.java | 9 + .../CacheServiceExceptionMapper.java | 23 ++- 8 files changed, 184 insertions(+), 130 deletions(-) create mode 100644 momento-sdk/src/intTest/java/momento/sdk/TestHelpers.java create mode 100644 momento-sdk/src/main/java/momento/sdk/exceptions/CacheNotFoundException.java diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 85451247..8f570c4e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,5 +31,5 @@ jobs: run: chmod +x gradlew - name: Build with Gradle run: ./gradlew clean build -# - name: Run integration tests -# run: ./gradlew integrationTest + - name: Run integration tests + run: ./gradlew integrationTest diff --git a/momento-sdk/src/intTest/java/momento/sdk/CacheTest.java b/momento-sdk/src/intTest/java/momento/sdk/CacheTest.java index 94ceab76..248370aa 100644 --- a/momento-sdk/src/intTest/java/momento/sdk/CacheTest.java +++ b/momento-sdk/src/intTest/java/momento/sdk/CacheTest.java @@ -3,8 +3,10 @@ */ package momento.sdk; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; +import static momento.sdk.TestHelpers.DEFAULT_CACHE_ENDPOINT; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator; import io.opentelemetry.context.propagation.ContextPropagators; @@ -13,26 +15,28 @@ import io.opentelemetry.sdk.trace.SdkTracerProvider; import io.opentelemetry.sdk.trace.export.BatchSpanProcessor; import java.io.BufferedReader; -import java.io.IOException; import java.io.InputStreamReader; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletionStage; -import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import momento.sdk.exceptions.CacheNotFoundException; +import momento.sdk.exceptions.ClientSdkException; +import momento.sdk.exceptions.PermissionDeniedException; import momento.sdk.messages.ClientGetResponse; import momento.sdk.messages.ClientSetResponse; import momento.sdk.messages.MomentoCacheResult; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; -class CacheTest { - Cache cache; +final class CacheTest { + + private Cache cache; @BeforeAll static void beforeAll() { @@ -59,7 +63,7 @@ Cache getCache(Optional openTelemetry) { Cache getCache(String authToken, String cacheName, Optional openTelemetry) { String endpoint = System.getenv("TEST_ENDPOINT"); if (endpoint == null) { - endpoint = "alpha.cacheservice.com"; + endpoint = DEFAULT_CACHE_ENDPOINT; } return new Cache( @@ -88,28 +92,22 @@ void testBlockingClientHappyPathWithTracing() throws Exception { verifyGetTrace("1"); } - void testHappyPath(Cache cache) { - try { - String key = UUID.randomUUID().toString(); - - // Set Key sync - ClientSetResponse setRsp = - cache.set(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 2); - Assertions.assertEquals(MomentoCacheResult.Ok, setRsp.getResult()); + private static void testHappyPath(Cache cache) { + String key = UUID.randomUUID().toString(); - // Get Key that was just set - ClientGetResponse rsp = cache.get(key); + // Set Key sync + ClientSetResponse setRsp = + cache.set(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 2); + assertEquals(MomentoCacheResult.Ok, setRsp.getResult()); - Assertions.assertEquals(MomentoCacheResult.Hit, rsp.getResult()); - Assertions.assertEquals("bar", StandardCharsets.US_ASCII.decode(rsp.getBody()).toString()); - - } catch (IOException e) { - Assertions.fail(e); - } + // Get Key that was just set + ClientGetResponse rsp = cache.get(key); + assertEquals(MomentoCacheResult.Hit, rsp.getResult()); + assertEquals("bar", StandardCharsets.US_ASCII.decode(rsp.getBody()).toString()); } @Test - void testAsyncClientHappyPath() { + void testAsyncClientHappyPath() throws Exception { testAsyncHappyPath(cache); } @@ -125,28 +123,22 @@ void testAsyncClientHappyPathWithTracing() throws Exception { verifyGetTrace("1"); } - void testAsyncHappyPath(Cache client) { - try { - String key = UUID.randomUUID().toString(); - // Set Key Async - CompletionStage setRsp = - client.setAsync(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 10); - Assertions.assertEquals( - MomentoCacheResult.Ok, setRsp.toCompletableFuture().get().getResult()); - - // Get Key Async - ClientGetResponse rsp = client.getAsync(key).toCompletableFuture().get(); + private static void testAsyncHappyPath(Cache client) throws Exception { + String key = UUID.randomUUID().toString(); + // Set Key Async + CompletionStage setRsp = + client.setAsync(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 10); + assertEquals(MomentoCacheResult.Ok, setRsp.toCompletableFuture().get().getResult()); - Assertions.assertEquals(MomentoCacheResult.Hit, rsp.getResult()); - Assertions.assertEquals("bar", StandardCharsets.US_ASCII.decode(rsp.getBody()).toString()); + // Get Key Async + ClientGetResponse rsp = client.getAsync(key).toCompletableFuture().get(); - } catch (IOException | InterruptedException | ExecutionException e) { - Assertions.fail(e); - } + assertEquals(MomentoCacheResult.Hit, rsp.getResult()); + assertEquals("bar", StandardCharsets.US_ASCII.decode(rsp.getBody()).toString()); } @Test - void testTtlHappyPath() { + void testTtlHappyPath() throws Exception { testTtlHappyPath(cache); } @@ -162,25 +154,19 @@ void testTtlHappyPathWithTracing() throws Exception { verifyGetTrace("1"); } - void testTtlHappyPath(Cache client) { - try { - String key = UUID.randomUUID().toString(); - - // Set Key sync - ClientSetResponse setRsp = - client.set(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 1); - Assertions.assertEquals(MomentoCacheResult.Ok, setRsp.getResult()); - - Thread.sleep(1500); + private static void testTtlHappyPath(Cache client) throws Exception { + String key = UUID.randomUUID().toString(); - // Get Key that was just set - ClientGetResponse rsp = client.get(key); + // Set Key sync + ClientSetResponse setRsp = + client.set(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 1); + assertEquals(MomentoCacheResult.Ok, setRsp.getResult()); - Assertions.assertEquals(MomentoCacheResult.Miss, rsp.getResult()); + Thread.sleep(1500); - } catch (IOException | InterruptedException e) { - Assertions.fail(e); - } + // Get Key that was just set + ClientGetResponse rsp = client.get(key); + assertEquals(MomentoCacheResult.Miss, rsp.getResult()); } @Test @@ -200,16 +186,11 @@ void testMissHappyPathWithTracing() throws Exception { verifyGetTrace("1"); } - void testMissHappyPathInternal(Cache client) { - try { - // Get Key that was just set - ClientGetResponse rsp = client.get(UUID.randomUUID().toString()); - - Assertions.assertEquals(MomentoCacheResult.Miss, rsp.getResult()); + private static void testMissHappyPathInternal(Cache client) { + // Get Key that was just set + ClientGetResponse rsp = client.get(UUID.randomUUID().toString()); - } catch (IOException e) { - Assertions.fail(e); - } + assertEquals(MomentoCacheResult.Miss, rsp.getResult()); } @Test @@ -226,31 +207,43 @@ void testBadAuthTokenWithTracing() throws Exception { testBadAuthToken(client); // To accommodate for delays in tracing logs to appear in docker Thread.sleep(1000); - verifySetTrace("0"); + verifySetTrace("1"); verifyGetTrace("1"); } - void testBadAuthToken(Cache badCredClient) { - - try { - // Get Key that was just set - ClientGetResponse rsp = badCredClient.get(UUID.randomUUID().toString()); + private static void testBadAuthToken(Cache badCredClient) { + // Bad Auth for Get + assertThrows(PermissionDeniedException.class, () -> badCredClient.get("myCacheKey")); + + // Bad Auth for Set + assertThrows( + PermissionDeniedException.class, + () -> + badCredClient.set( + "myCacheKey", + ByteBuffer.wrap("cache me if you can".getBytes(StandardCharsets.UTF_8)), + 500)); + } - Assertions.fail("expected PERMISSION_DENIED io.grpc.StatusRuntimeException"); + @Test + public void unreachableEndpoint_ThrowsException() { + Cache cache = + new Cache( + System.getenv("TEST_AUTH_TOKEN"), + System.getenv("TEST_CACHE_NAME"), + "nonexistent.preprod.a.momentohq.com"); + + assertThrows(ClientSdkException.class, () -> cache.get("key")); + } - } catch (IOException e) { - Assertions.fail(e); - } catch (io.grpc.StatusRuntimeException e) { + @Disabled("TODO: Update to catch cache not ready and then do a get again to see not found.") + @Test + public void invalidCache_ThrowsNotFoundException() { + Cache cache = + getCache(System.getenv("TEST_AUTH_TOKEN"), UUID.randomUUID().toString(), Optional.empty()); - // Make sure we get permission denied error the way we would expected - Assertions.assertEquals( - new StatusRuntimeException( - Status.PERMISSION_DENIED.withDescription("Malformed authorization token")) - .toString(), - e.toString()); - } + assertThrows(CacheNotFoundException.class, () -> cache.get("key")); } - /** ================ HELPER FUNCTIONS ====================================== */ OpenTelemetrySdk setOtelSDK() { String otelGwUrl = "0.0.0.0"; @@ -305,7 +298,7 @@ void verifySetTrace(String expectedCount) throws Exception { true, "Verify set trace", "failed to verify set trace"); - Assertions.assertEquals(expectedCount, count.trim()); + assertEquals(expectedCount, count.trim()); } void verifyGetTrace(String expectedCount) throws Exception { @@ -315,7 +308,7 @@ void verifyGetTrace(String expectedCount) throws Exception { true, "Verify get trace", "failed to verify get trace"); - Assertions.assertEquals(expectedCount, count.trim()); + assertEquals(expectedCount, count.trim()); } // Polls a command until the expected result comes back diff --git a/momento-sdk/src/intTest/java/momento/sdk/MomentoTest.java b/momento-sdk/src/intTest/java/momento/sdk/MomentoTest.java index 473a436b..a20a7b59 100644 --- a/momento-sdk/src/intTest/java/momento/sdk/MomentoTest.java +++ b/momento-sdk/src/intTest/java/momento/sdk/MomentoTest.java @@ -3,17 +3,25 @@ */ package momento.sdk; +import static momento.sdk.TestHelpers.DEFAULT_MOMENTO_HOSTED_ZONE_ENDPOINT; +import static org.junit.jupiter.api.Assertions.assertEquals; + import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import momento.sdk.exceptions.CacheAlreadyExistsException; import momento.sdk.messages.ClientGetResponse; import momento.sdk.messages.ClientSetResponse; import momento.sdk.messages.MomentoCacheResult; -import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +final class MomentoTest { -class MomentoTest { - // Cache cache; + private String authToken; + private String cacheName; - // @org.junit.jupiter.api.BeforeAll + @BeforeAll static void beforeAll() { if (System.getenv("TEST_AUTH_TOKEN") == null) { throw new IllegalArgumentException( @@ -25,31 +33,40 @@ static void beforeAll() { } } - @org.junit.jupiter.api.Test - void testHappyPath() { - try { - Momento m = - Momento.builder() - .authToken(System.getenv("TEST_AUTH_TOKEN")) - .endpointOverride("cell-alpha-dev.preprod.a.momentohq.com") - .build(); - Cache cache = m.createCache(System.getenv("TEST_CACHE_NAME")); + @BeforeEach + void setup() { + this.authToken = System.getenv("TEST_AUTH_TOKEN"); + this.cacheName = System.getenv("TEST_CACHE_NAME"); + } - String key = java.util.UUID.randomUUID().toString(); + @Test + void testHappyPath() { + Momento momento = + Momento.builder() + .authToken(authToken) + .endpointOverride(DEFAULT_MOMENTO_HOSTED_ZONE_ENDPOINT) + .build(); + Cache cache = getOrCreate(momento, cacheName); - // Set Key sync - ClientSetResponse setRsp = - cache.set(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 2); - Assertions.assertEquals(MomentoCacheResult.Ok, setRsp.getResult()); + String key = java.util.UUID.randomUUID().toString(); - // Get Key that was just set - ClientGetResponse rsp = cache.get(key); + // Set Key sync + ClientSetResponse setRsp = + cache.set(key, ByteBuffer.wrap("bar".getBytes(StandardCharsets.UTF_8)), 2); + assertEquals(MomentoCacheResult.Ok, setRsp.getResult()); - Assertions.assertEquals(MomentoCacheResult.Hit, rsp.getResult()); - Assertions.assertEquals("bar", StandardCharsets.US_ASCII.decode(rsp.getBody()).toString()); + // Get Key that was just set + ClientGetResponse rsp = cache.get(key); + assertEquals(MomentoCacheResult.Hit, rsp.getResult()); + assertEquals("bar", StandardCharsets.US_ASCII.decode(rsp.getBody()).toString()); + } - } catch (java.io.IOException e) { - Assertions.fail(e); + // TODO: Update this to be recreated each time and add a separate test case for Already Exists + private static Cache getOrCreate(Momento momento, String cacheName) { + try { + return momento.createCache(cacheName); + } catch (CacheAlreadyExistsException e) { + return momento.getCache(cacheName); } } } diff --git a/momento-sdk/src/intTest/java/momento/sdk/TestHelpers.java b/momento-sdk/src/intTest/java/momento/sdk/TestHelpers.java new file mode 100644 index 00000000..7d1ef23e --- /dev/null +++ b/momento-sdk/src/intTest/java/momento/sdk/TestHelpers.java @@ -0,0 +1,11 @@ +package momento.sdk; + +public final class TestHelpers { + + private TestHelpers() {} + + public static final String DEFAULT_MOMENTO_HOSTED_ZONE_ENDPOINT = + "cell-alpha-dev.preprod.a.momentohq.com"; + public static final String DEFAULT_CACHE_ENDPOINT = + "cache." + DEFAULT_MOMENTO_HOSTED_ZONE_ENDPOINT; +} diff --git a/momento-sdk/src/main/java/momento/sdk/Cache.java b/momento-sdk/src/main/java/momento/sdk/Cache.java index c78e0dab..2dfc6e3b 100644 --- a/momento-sdk/src/main/java/momento/sdk/Cache.java +++ b/momento-sdk/src/main/java/momento/sdk/Cache.java @@ -35,6 +35,8 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import javax.net.ssl.SSLException; +import momento.sdk.exceptions.CacheServiceExceptionMapper; +import momento.sdk.exceptions.ClientSdkException; import momento.sdk.messages.ClientGetResponse; import momento.sdk.messages.ClientSetResponse; @@ -136,7 +138,7 @@ public Cache( * @return {@link ClientGetResponse} with the response object as a {@link java.nio.ByteBuffer} * @throws IOException if an error occurs opening input stream for response body. */ - public ClientGetResponse get(String key) throws IOException { + public ClientGetResponse get(String key) { Optional span = buildSpan("java-sdk-get-request"); try (Scope ignored = (span.map(ImplicitContextKeyed::makeCurrent).orElse(null))) { GetResponse rsp = blockingStub.get(buildGetRequest(key)); @@ -152,8 +154,7 @@ public ClientGetResponse get(String key) throws IOException { theSpan.recordException(e); theSpan.setStatus(StatusCode.ERROR); }); - // TODO - Yup I know this is ugly but we have work to do to handle excpetions properly. - throw e; + throw CacheServiceExceptionMapper.convert(e); } finally { span.ifPresent(theSpan -> theSpan.end(now())); } @@ -170,7 +171,7 @@ public ClientGetResponse get(String key) throws IOException { * @return {@link ClientSetResponse} with the result of the set operation * @throws IOException if an error occurs opening ByteBuffer for request body. */ - public ClientSetResponse set(String key, ByteBuffer value, int ttlSeconds) throws IOException { + public ClientSetResponse set(String key, ByteBuffer value, int ttlSeconds) { Optional span = buildSpan("java-sdk-set-request"); try (Scope ignored = (span.map(ImplicitContextKeyed::makeCurrent).orElse(null))) { SetResponse rsp = blockingStub.set(buildSetRequest(key, value, ttlSeconds * 1000)); @@ -184,8 +185,7 @@ public ClientSetResponse set(String key, ByteBuffer value, int ttlSeconds) throw theSpan.recordException(e); theSpan.setStatus(StatusCode.ERROR); }); - // TODO - Yup I know this is ugly, but we have work to do to handle exceptions. - throw e; + throw CacheServiceExceptionMapper.convert(e); } finally { span.ifPresent(theSpan -> theSpan.end(now())); } @@ -234,6 +234,7 @@ public void onSuccess(GetResponse rsp) { scope.ifPresent(Scope::close); } + // TODO: Handle Exception Mapping @Override public void onFailure(Throwable e) { returnFuture.completeExceptionally(e); @@ -264,8 +265,7 @@ public void onFailure(Throwable e) { * CompletionStage interface wrapping standard ClientSetResponse. * @throws IOException if an error occurs opening ByteBuffer for request body. */ - public CompletionStage setAsync(String key, ByteBuffer value, int ttlSeconds) - throws IOException { + public CompletionStage setAsync(String key, ByteBuffer value, int ttlSeconds) { Optional span = buildSpan("java-sdk-set-request"); Optional scope = (span.map(ImplicitContextKeyed::makeCurrent)); @@ -301,6 +301,7 @@ public void onSuccess(SetResponse rsp) { scope.ifPresent(Scope::close); } + // TODO: Handle Exception Mapping @Override public void onFailure(Throwable e) { returnFuture.completeExceptionally(e); // bubble all errors up @@ -330,12 +331,16 @@ private GetRequest buildGetRequest(String key) { .build(); } - private SetRequest buildSetRequest(String key, ByteBuffer value, int ttl) throws IOException { - return SetRequest.newBuilder() - .setCacheKey(ByteString.copyFrom(key, StandardCharsets.UTF_8)) - .setCacheBody(ByteString.readFrom(new ByteArrayInputStream(value.array()))) - .setTtlMilliseconds(ttl) - .build(); + private SetRequest buildSetRequest(String key, ByteBuffer value, int ttl) { + try { + return SetRequest.newBuilder() + .setCacheKey(ByteString.copyFrom(key, StandardCharsets.UTF_8)) + .setCacheBody(ByteString.readFrom(new ByteArrayInputStream(value.array()))) + .setTtlMilliseconds(ttl) + .build(); + } catch (IOException e) { + throw new ClientSdkException("Failed to create request."); + } } private Optional buildSpan(String spanName) { diff --git a/momento-sdk/src/main/java/momento/sdk/Momento.java b/momento-sdk/src/main/java/momento/sdk/Momento.java index 958da9a1..8709487e 100644 --- a/momento-sdk/src/main/java/momento/sdk/Momento.java +++ b/momento-sdk/src/main/java/momento/sdk/Momento.java @@ -57,7 +57,7 @@ public Cache createCache(String cacheName) { this.blockingStub.createCache(buildCreateCacheRequest(cacheName)); return makeCacheClient(authToken, cacheName, hostedZone); } catch (io.grpc.StatusRuntimeException e) { - if (e.getStatus() == Status.ALREADY_EXISTS) { + if (e.getStatus().getCode() == Status.Code.ALREADY_EXISTS) { throw new CacheAlreadyExistsException( String.format("Cache with name %s already exists", cacheName)); } diff --git a/momento-sdk/src/main/java/momento/sdk/exceptions/CacheNotFoundException.java b/momento-sdk/src/main/java/momento/sdk/exceptions/CacheNotFoundException.java new file mode 100644 index 00000000..e87893d5 --- /dev/null +++ b/momento-sdk/src/main/java/momento/sdk/exceptions/CacheNotFoundException.java @@ -0,0 +1,9 @@ +package momento.sdk.exceptions; + +/** Exception when operations are performed on a Cache that doesn't exist. */ +public class CacheNotFoundException extends CacheServiceException { + + public CacheNotFoundException(String message) { + super(message); + } +} diff --git a/momento-sdk/src/main/java/momento/sdk/exceptions/CacheServiceExceptionMapper.java b/momento-sdk/src/main/java/momento/sdk/exceptions/CacheServiceExceptionMapper.java index 1c345406..68659c09 100644 --- a/momento-sdk/src/main/java/momento/sdk/exceptions/CacheServiceExceptionMapper.java +++ b/momento-sdk/src/main/java/momento/sdk/exceptions/CacheServiceExceptionMapper.java @@ -1,9 +1,14 @@ package momento.sdk.exceptions; +import io.grpc.Status; import io.grpc.StatusRuntimeException; +import java.net.UnknownHostException; public final class CacheServiceExceptionMapper { + private static final String INTERNAL_SERVER_ERROR_MESSAGE = + "Unexpected exception occurred while trying to fulfill the request."; + private CacheServiceExceptionMapper() {} /** @@ -24,12 +29,26 @@ public static SdkException convert(Exception e) { case PERMISSION_DENIED: return new PermissionDeniedException(grpcException.getMessage()); + case NOT_FOUND: + return new CacheNotFoundException(grpcException.getMessage()); + default: - return new InternalServerException( - "Unexpected exception occurred while trying to fulfill the request."); + if (isDnsUnreachable(grpcException)) { + return new ClientSdkException( + String.format( + "Unable to reach request endpoint. Request failed with %s", + grpcException.getMessage())); + } + return new InternalServerException(INTERNAL_SERVER_ERROR_MESSAGE); } } return new ClientSdkException("SDK Failed to process the request", e); } + + private static boolean isDnsUnreachable(StatusRuntimeException e) { + return e.getStatus().getCode() == Status.Code.UNAVAILABLE + && e.getCause() instanceof RuntimeException + && e.getCause().getCause() instanceof UnknownHostException; + } }