bbotella commented on code in PR #152:
URL: https://github.com/apache/cassandra-sidecar/pull/152#discussion_r1868498963
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java:
##########
@@ -331,4 +334,34 @@ public static String extractHostAddressWithoutPort(String
address)
}
return address;
}
+
+ protected StorageOperations getStorageOperations(String host)
+ {
+ CassandraAdapterDelegate delegate =
this.metadataFetcher.delegate(host);
+ StorageOperations storageOperations = delegate == null ? null :
delegate.storageOperations();
+ if (storageOperations == null)
+ {
+ throw cassandraServiceUnavailable();
+ }
+
+
Review Comment:
Extra space
##########
server/src/test/java/org/apache/cassandra/sidecar/routes/cassandra/GetPreemptiveOpenIntervalHandlerTest.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.sidecar.routes.cassandra;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import io.vertx.core.buffer.Buffer;
+import io.vertx.core.json.JsonObject;
+import io.vertx.ext.web.client.HttpResponse;
+import io.vertx.ext.web.client.WebClient;
+import io.vertx.ext.web.client.predicate.ResponsePredicate;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import
org.apache.cassandra.sidecar.common.response.GetPreemptiveOpenIntervalResponse;
+
+import static
io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR;
+import static io.netty.handler.codec.http.HttpResponseStatus.OK;
+import static
io.netty.handler.codec.http.HttpResponseStatus.SERVICE_UNAVAILABLE;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(VertxExtension.class)
+class GetPreemptiveOpenIntervalHandlerTest extends JmxCommonTest
+{
+ private static final String testRoute =
"/api/v1/cassandra/sstable/preemptive-open-interval";
+
+ @Test
+ void testWithoutInstanceId(VertxTestContext context)
+ {
+ when(storageOperations.getSSTablePreemptiveOpenIntervalInMB())
+ .thenReturn(new GetPreemptiveOpenIntervalResponse(20));
+
+ WebClient client = WebClient.create(vertx);
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> verifyResponse(context,
+ response,
+ "20")));
+ }
+
+ @Test
+ void testWithInstanceId(VertxTestContext context)
+ {
+ when(storageOperations.getSSTablePreemptiveOpenIntervalInMB())
+ .thenReturn(new GetPreemptiveOpenIntervalResponse(10));
+
+ WebClient client = WebClient.create(vertx);
+ client.get(server.actualPort(), "127.0.0.1", testRoute +
"?instanceId=200")
+ .expect(ResponsePredicate.SC_OK)
+ .send(context.succeeding(response -> verifyResponse(context,
+ response,
+ "10")));
+ }
+
+ @Test
+ void testFailure(VertxTestContext context)
+ {
+ doThrow(new
RuntimeException()).when(storageOperations).getSSTablePreemptiveOpenIntervalInMB();
+
+ WebClient client = WebClient.create(vertx);
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_INTERNAL_SERVER_ERROR)
+ .send(context.succeeding(response -> {
+
assertThat(response.statusCode()).isEqualTo(INTERNAL_SERVER_ERROR.code());
+ context.completeNow();
+ }));
+ }
+
+ @Test
+ void testNullStorageOps(VertxTestContext context)
+ {
+ when(delegate.storageOperations()).thenReturn(null);
+
+ WebClient client = WebClient.create(vertx);
+ client.get(server.actualPort(), "127.0.0.1", testRoute)
+ .expect(ResponsePredicate.SC_SERVICE_UNAVAILABLE)
+ .send(context.succeeding(response -> {
+
assertThat(response.statusCode()).isEqualTo(SERVICE_UNAVAILABLE.code());
+ context.completeNow();
+ }));
+ }
+
+ private void verifyResponse(VertxTestContext context, HttpResponse<Buffer>
response, String expectedValue)
Review Comment:
Nit: verifyValidResponse?
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/ApiEndpointsV1.java:
##########
@@ -118,6 +118,11 @@ public final class ApiEndpointsV1
public static final String CONNECTED_CLIENT_STATS_ROUTE = API_V1 +
CASSANDRA + "/stats/connected-clients";
+ // Endpoint to retrieve sstable's preemptiveOpenInterval value.
+ // Value returned is in MB, may return negative value when disabled
+ private static final String SSTABLE = "/sstable";
+ public static final String SSTABLE_PREEMPTIVE_OPEN_INTERVAL_ROUTE = API_V1
+ CASSANDRA + SSTABLE +
Review Comment:
I know we are reusing the `getSsTablePreemtiveOpenIntervalInMB` JMX
operation, but, from a pure API standpoint, we are omitting the `MB` part in
the endpoint, but returning it as part of the response
(`SSTablePreemptiveOpenIntervalInMB`). My suggestion for this would be to
either:
- We add the `MB` part to the endpoint (`/preemptive-open-interval-in-mb`)
Or
- We just support an optional `unit` filtering parameter in the url, that
can be extended in the future with other unit types if needed. That way, the
url would end in `/preemptive-open-interval?unit=mb` (remember that the unit is
optional with MB as a default, so no need to pass it from the client).
That would make us have to modify the response to something like:
```
{
"SSTablePreemptiveOpenInterval": 1234,
"unit": "MB"
}
```
What do you think?
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java:
##########
@@ -331,4 +334,34 @@ public static String extractHostAddressWithoutPort(String
address)
}
return address;
}
+
+ protected StorageOperations getStorageOperations(String host)
+ {
+ CassandraAdapterDelegate delegate =
this.metadataFetcher.delegate(host);
+ StorageOperations storageOperations = delegate == null ? null :
delegate.storageOperations();
+ if (storageOperations == null)
+ {
+ throw cassandraServiceUnavailable();
Review Comment:
Do we want to add some text to this error?
##########
server/src/main/java/org/apache/cassandra/sidecar/metrics/ServerMetrics.java:
##########
@@ -53,4 +53,9 @@ public interface ServerMetrics
* @return metrics related to internal caches that are tracked.
*/
CacheMetrics cacheMetrics();
+
+ /**
+ * @return metrics related to invocation of C* JMX operations
+ */
+ JmxOperationsMetrics jmxOperationsMetrics();
Review Comment:
Should these Jmx operations metrics be part of a different PR?
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/AbstractHandler.java:
##########
@@ -331,4 +334,34 @@ public static String extractHostAddressWithoutPort(String
address)
}
return address;
}
+
+ protected StorageOperations getStorageOperations(String host)
+ {
+ CassandraAdapterDelegate delegate =
this.metadataFetcher.delegate(host);
+ StorageOperations storageOperations = delegate == null ? null :
delegate.storageOperations();
+ if (storageOperations == null)
+ {
+ throw cassandraServiceUnavailable();
+ }
+
+
+ return storageOperations;
+ }
+
+ protected <V> void updateJmxMetric(AsyncResult<V> result,
+ JmxOperationsMetrics
jmxOperationsMetrics,
+ String operationName,
+ long startTime)
+ {
+ if (result.succeeded())
+ {
+ jmxOperationsMetrics.recordTimeTaken(operationName + "Succeeded",
Review Comment:
For the record time taken method, I think we could explore the possibility
of splitting out the `Succeeded`/`Failed` into its own Status `parameter`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
