You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/11/10 21:08:07 UTC
[GitHub] [cassandra-sidecar] jyothsnakonisa opened a new pull request, #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
jyothsnakonisa opened a new pull request, #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42
Adds new APIs
GET /api/v1/cassandra/status
- Returns the NodeStatus with release version & partitioner info of the node receiving the request GET /api/v1/cassandra/status?instanceId={id}
- Returs the NodeStatus with release version & partitioner info of a given instanceId
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] nvharikrishna commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
nvharikrishna commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1023158706
##########
common/src/main/java/org/apache/cassandra/sidecar/common/NodeStatus.java:
##########
@@ -19,10 +19,39 @@
package org.apache.cassandra.sidecar.common;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
/**
- * Placeholder
+ * Holds information about the Status of a node
*/
public class NodeStatus
{
+ private final String releaseVersion;
+ private final String partitioner;
+
+ /**
+ * Constructs a new {@link NodeStatus} object with the Cassandra node's release version and partitioner
+ * information.
+ *
+ * @param releaseVersion the release version of the Cassandra node
+ * @param partitioner the partitioner used by the Cassandra node
+ */
+ public NodeStatus(@JsonProperty("releaseVersion") String releaseVersion,
+ @JsonProperty("partitioner") String partitioner)
Review Comment:
Neither releaseVersion nor partitoner says node is up or down. Why this class is called as NodeStatus?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
yifan-c commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1022102105
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
+ description = "Returns HTTP 200 if Cassandra is available, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/status")
+ public Response status(@Context HttpServerRequest req,
+ @QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
+ {
+ NodeStatus nodeStatus;
+ try
{
- cassandra = metadataFetcher.getDelegate(instanceId);
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ nodeStatus = cassandra.getStatus();
}
- else
+ catch (IllegalArgumentException e)
Review Comment:
Should all `IllegalArgumentException`s be mapped to 400? It might not hold true for all.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] jyothsnakonisa commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
jyothsnakonisa commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1022000034
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
+ description = "Returns HTTP 200 if Cassandra is available, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/status")
+ public Response status(@Context HttpServerRequest req,
+ @QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
+ {
+ NodeStatus nodeStatus;
+ try
{
- cassandra = metadataFetcher.getDelegate(instanceId);
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ nodeStatus = cassandra.getStatus();
}
- else
+ catch (IllegalArgumentException e)
Review Comment:
If the instanceId is incorrect then an IllegalArgumentException is being thrown. You can refer the test case `validRequestWithInvalidInstanceId `. Without this, we would throw internal server error to the client, which I think is incorrect.
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
+ description = "Returns HTTP 200 if Cassandra is available, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/status")
+ public Response status(@Context HttpServerRequest req,
+ @QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
+ {
+ NodeStatus nodeStatus;
+ try
{
- cassandra = metadataFetcher.getDelegate(instanceId);
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ nodeStatus = cassandra.getStatus();
}
- else
+ catch (IllegalArgumentException e)
Review Comment:
@frankgh If the instanceId is incorrect then an IllegalArgumentException is being thrown. You can refer the test case `validRequestWithInvalidInstanceId `. Without this, we would throw internal server error to the client, which I think is incorrect.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1024293015
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraSettingsService.java:
##########
@@ -0,0 +1,66 @@
+package org.apache.cassandra.sidecar.routes;
Review Comment:
can we add the license header to this file?
##########
src/test/java/org/apache/cassandra/sidecar/routes/CassandraSettingsServiceTest.java:
##########
@@ -0,0 +1,118 @@
+package org.apache.cassandra.sidecar.routes;
Review Comment:
can we add the license header to this file as well
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] jyothsnakonisa commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
jyothsnakonisa commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1024321767
##########
src/test/java/org/apache/cassandra/sidecar/routes/CassandraSettingsServiceTest.java:
##########
@@ -0,0 +1,118 @@
+package org.apache.cassandra.sidecar.routes;
Review Comment:
added license information.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] nvharikrishna commented on pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
nvharikrishna commented on PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#issuecomment-1319060811
The comment I have is not related to the context of this PR. Don't want to block you on that. LGTM from my side.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1020523157
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
+ description = "Returns HTTP 200 if Cassandra is available, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/status")
Review Comment:
does settings make more sense?
```suggestion
@Path("/v1/cassandra/settings")
```
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
Review Comment:
the name of the class `CassandraHealthService` doesn't make sense anymore if we add another endpoint here. We should either rename the class, or add a new class for the new endpoint.
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
+ description = "Returns HTTP 200 if Cassandra is available, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/status")
+ public Response status(@Context HttpServerRequest req,
+ @QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
+ {
+ NodeStatus nodeStatus;
+ try
{
- cassandra = metadataFetcher.getDelegate(instanceId);
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ nodeStatus = cassandra.getStatus();
}
- else
+ catch (IllegalArgumentException e)
Review Comment:
I don't know if this try/catch block is necessary, I am not able to pin point exactly where the `IllegalArgumentException` would come from
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1022036661
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraHealthService.java:
##########
@@ -68,16 +71,35 @@ public CassandraHealthService(InstanceMetadataFetcher metadataFetcher)
public Response getCassandraHealth(@Context HttpServerRequest req,
@QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
{
- CassandraAdapterDelegate cassandra;
- if (instanceId != null)
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ return getHealthResponse(cassandra);
+ }
+
+ @Operation(summary = "Cassandra's node status",
+ description = "Returns HTTP 200 if Cassandra is available, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/status")
+ public Response status(@Context HttpServerRequest req,
+ @QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
+ {
+ NodeStatus nodeStatus;
+ try
{
- cassandra = metadataFetcher.getDelegate(instanceId);
+ CassandraAdapterDelegate cassandra = getCassandraAdapterDelegate(req.host(), instanceId);
+ nodeStatus = cassandra.getStatus();
}
- else
+ catch (IllegalArgumentException e)
Review Comment:
should we instead register an exception mapper to handle `IllegalArgumentException` errors?
```java
public class IllegalArgumentExceptionMapper implements ExceptionMapper<IllegalArgumentException>
{
@Override
public Response toResponse(IllegalArgumentException exception)
{
return Response.status(Response.Status.BAD_REQUEST)
...
}
}
```
and then inside `org.apache.cassandra.sidecar.MainModule#configureServices()` we can register it
```
resteasyDeployment.getProviderFactory().registerProvider(IllegalArgumentExceptionMapper.class);
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] nvharikrishna commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
nvharikrishna commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1024404608
##########
common/src/main/java/org/apache/cassandra/sidecar/common/ICassandraAdapter.java:
##########
@@ -28,6 +26,6 @@
*/
public interface ICassandraAdapter
{
- List<NodeStatus> getStatus();
+ NodeSettings getStatus();
Review Comment:
Now this method won't become getSettings() ?
Probably, this could be something like this:
```
boolean getStatus();
NodeSettings getSettings();
```
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraSettingsService.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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;
+
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import io.netty.handler.codec.http.HttpResponseStatus;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.responses.ApiResponse;
+import io.vertx.core.http.HttpServerRequest;
+import org.apache.cassandra.sidecar.common.CassandraAdapterDelegate;
+import org.apache.cassandra.sidecar.common.NodeSettings;
+import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher;
+
+
+/**
+ * Provides REST endpoint to get the configured settings of a cassandra node
+ */
+@Singleton
+@Path("/api")
+@Produces(MediaType.APPLICATION_JSON)
+public class CassandraSettingsService
+{
+ private final InstanceMetadataFetcher metadataFetcher;
+
+ @Inject
+ public CassandraSettingsService(InstanceMetadataFetcher metadataFetcher)
+ {
+ this.metadataFetcher = metadataFetcher;
+ }
+
+ @Operation(summary = "Cassandra's node settings",
+ description = "Returns HTTP 200 if Cassandra is available,400 on incorrect instanceId, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "400", description = "Incorrect instanceId"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/settings")
+ public Response status(@Context HttpServerRequest req,
Review Comment:
Shall we rename this method too (settings instead of status)?
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraSettingsService.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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;
+
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import io.netty.handler.codec.http.HttpResponseStatus;
+import io.swagger.v3.oas.annotations.Operation;
+import io.swagger.v3.oas.annotations.responses.ApiResponse;
+import io.vertx.core.http.HttpServerRequest;
+import org.apache.cassandra.sidecar.common.CassandraAdapterDelegate;
+import org.apache.cassandra.sidecar.common.NodeSettings;
+import org.apache.cassandra.sidecar.utils.InstanceMetadataFetcher;
+
+
+/**
+ * Provides REST endpoint to get the configured settings of a cassandra node
+ */
+@Singleton
+@Path("/api")
+@Produces(MediaType.APPLICATION_JSON)
+public class CassandraSettingsService
+{
+ private final InstanceMetadataFetcher metadataFetcher;
+
+ @Inject
+ public CassandraSettingsService(InstanceMetadataFetcher metadataFetcher)
+ {
+ this.metadataFetcher = metadataFetcher;
+ }
+
+ @Operation(summary = "Cassandra's node settings",
+ description = "Returns HTTP 200 if Cassandra is available,400 on incorrect instanceId, 503 otherwise",
+ responses = {
+ @ApiResponse(responseCode = "200", description = "Cassandra status is available"),
+ @ApiResponse(responseCode = "400", description = "Incorrect instanceId"),
+ @ApiResponse(responseCode = "503", description = "Cassandra status is not available")
+ })
+ @GET
+ @Path("/v1/cassandra/settings")
+ public Response status(@Context HttpServerRequest req,
+ @QueryParam(AbstractHandler.INSTANCE_ID) Integer instanceId)
+ {
+ NodeSettings nodeSettings;
+ try
+ {
+ CassandraAdapterDelegate cassandra = metadataFetcher.getDelegate(req.host(), instanceId);
+ nodeSettings = cassandra.getStatus();
+ }
+ catch (IllegalArgumentException e)
+ {
+ return Response.status(HttpResponseStatus.BAD_REQUEST.code()).entity(e.getMessage()).build();
+ }
+ int statusCode = nodeSettings != null
+ ? HttpResponseStatus.OK.code()
+ : HttpResponseStatus.SERVICE_UNAVAILABLE.code();
Review Comment:
:+1:
##########
common/src/main/java/org/apache/cassandra/sidecar/common/CassandraAdapterDelegate.java:
##########
@@ -133,45 +133,46 @@ private void healthCheckInternal()
if (activeSession == null)
{
logger.info("No local CQL session is available. Cassandra is down presumably.");
- isUp = false;
+ status = null;
return;
}
maybeRegisterHostListener(activeSession);
try
{
- String version = activeSession.execute("select release_version from system.local")
- .one()
- .getString("release_version");
- isUp = true;
+ Row oneResult = activeSession.execute("select release_version, partitioner from system.local")
+ .one();
+ NodeSettings nodeSettings = new NodeSettings(oneResult.getString("release_version"),
+ oneResult.getString("partitioner"));
+ this.status = nodeSettings;
// this might swap the adapter out
- SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(version);
+ SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(nodeSettings.releaseVersion());
if (!newVersion.equals(currentVersion))
{
currentVersion = newVersion;
- adapter = versionProvider.getCassandra(version).create(cqlSession);
+ adapter = versionProvider.getCassandra(nodeSettings.releaseVersion()).create(cqlSession);
logger.info("Cassandra version change detected. New adapter loaded: {}", adapter);
}
- logger.debug("Cassandra version {}", version);
+ logger.debug("Cassandra version {}", nodeSettings.releaseVersion());
}
catch (NoHostAvailableException e)
{
logger.error("Unexpected error connecting to Cassandra instance.", e);
// The cassandra node is down.
// Unregister the host listener and nullify the session in order to get a new object.
- isUp = false;
+ status = null;
maybeUnregisterHostListener(activeSession);
session = null;
Review Comment:
On a different note, doesn't it make sense to close the session in this case?
##########
common/src/main/java/org/apache/cassandra/sidecar/common/CassandraAdapterDelegate.java:
##########
@@ -50,7 +50,7 @@ public class CassandraAdapterDelegate implements ICassandraAdapter, Host.StateLi
private volatile Session session;
private SimpleCassandraVersion currentVersion;
private ICassandraAdapter adapter;
- private volatile boolean isUp = false;
+ private volatile NodeSettings status = null;
Review Comment:
Can the status be renamed to settings or nodeSettings? Doesn't it make sense to keep both of them (isUP and nodeSettings)?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1026682748
##########
common/src/main/java/org/apache/cassandra/sidecar/common/CassandraAdapterDelegate.java:
##########
@@ -133,45 +133,47 @@ private void healthCheckInternal()
if (activeSession == null)
{
logger.info("No local CQL session is available. Cassandra is down presumably.");
- isUp = false;
+ nodeSettings = null;
return;
}
maybeRegisterHostListener(activeSession);
try
{
- String version = activeSession.execute("select release_version from system.local")
- .one()
- .getString("release_version");
- isUp = true;
+ Row oneResult = activeSession.execute("select release_version, partitioner from system.local")
+ .one();
+ NodeSettings nodeSettings = new NodeSettings(oneResult.getString("release_version"),
+ oneResult.getString("partitioner"));
+ this.nodeSettings = nodeSettings;
// this might swap the adapter out
- SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(version);
+ SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(nodeSettings.releaseVersion());
if (!newVersion.equals(currentVersion))
{
currentVersion = newVersion;
- adapter = versionProvider.getCassandra(version).create(cqlSession);
+ adapter = versionProvider.getCassandra(nodeSettings.releaseVersion()).create(cqlSession);
logger.info("Cassandra version change detected. New adapter loaded: {}", adapter);
}
- logger.debug("Cassandra version {}", version);
+ logger.debug("Cassandra version {}", nodeSettings.releaseVersion());
}
catch (NoHostAvailableException e)
{
logger.error("Unexpected error connecting to Cassandra instance.", e);
// The cassandra node is down.
// Unregister the host listener and nullify the session in order to get a new object.
- isUp = false;
+ nodeSettings = null;
maybeUnregisterHostListener(activeSession);
+ session.close();
Review Comment:
we probably need to wrap this in a try/catch block , errors coming from the session should be safely ignored. And we probably want `activeSession` here instead of `session`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1024444661
##########
common/src/main/java/org/apache/cassandra/sidecar/common/ICassandraAdapter.java:
##########
@@ -28,6 +26,6 @@
*/
public interface ICassandraAdapter
{
- List<NodeStatus> getStatus();
+ NodeSettings getStatus();
Review Comment:
or just `settings()` following Cassandra conventions
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
frankgh commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1023403637
##########
common/src/main/java/org/apache/cassandra/sidecar/common/NodeSettings.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.common;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+/**
+ * Holds information about the Status of a node
Review Comment:
```suggestion
* Holds information about specific node settings
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c closed pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
yifan-c closed pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
URL: https://github.com/apache/cassandra-sidecar/pull/42
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
yifan-c commented on PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#issuecomment-1334542007
It is already committed. Closing the PR
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] jyothsnakonisa commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
jyothsnakonisa commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1024321560
##########
src/main/java/org/apache/cassandra/sidecar/routes/CassandraSettingsService.java:
##########
@@ -0,0 +1,66 @@
+package org.apache.cassandra.sidecar.routes;
Review Comment:
Added the license information.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] jyothsnakonisa commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
jyothsnakonisa commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1024453073
##########
common/src/main/java/org/apache/cassandra/sidecar/common/CassandraAdapterDelegate.java:
##########
@@ -133,45 +133,46 @@ private void healthCheckInternal()
if (activeSession == null)
{
logger.info("No local CQL session is available. Cassandra is down presumably.");
- isUp = false;
+ status = null;
return;
}
maybeRegisterHostListener(activeSession);
try
{
- String version = activeSession.execute("select release_version from system.local")
- .one()
- .getString("release_version");
- isUp = true;
+ Row oneResult = activeSession.execute("select release_version, partitioner from system.local")
+ .one();
+ NodeSettings nodeSettings = new NodeSettings(oneResult.getString("release_version"),
+ oneResult.getString("partitioner"));
+ this.status = nodeSettings;
// this might swap the adapter out
- SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(version);
+ SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(nodeSettings.releaseVersion());
if (!newVersion.equals(currentVersion))
{
currentVersion = newVersion;
- adapter = versionProvider.getCassandra(version).create(cqlSession);
+ adapter = versionProvider.getCassandra(nodeSettings.releaseVersion()).create(cqlSession);
logger.info("Cassandra version change detected. New adapter loaded: {}", adapter);
}
- logger.debug("Cassandra version {}", version);
+ logger.debug("Cassandra version {}", nodeSettings.releaseVersion());
}
catch (NoHostAvailableException e)
{
logger.error("Unexpected error connecting to Cassandra instance.", e);
// The cassandra node is down.
// Unregister the host listener and nullify the session in order to get a new object.
- isUp = false;
+ status = null;
maybeUnregisterHostListener(activeSession);
session = null;
Review Comment:
@nvharikrishna I think we want to keep the session open as long as the node is running, @frankgh correct me if I am wrong.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] nvharikrishna commented on a diff in pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
nvharikrishna commented on code in PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#discussion_r1025574393
##########
common/src/main/java/org/apache/cassandra/sidecar/common/CassandraAdapterDelegate.java:
##########
@@ -133,45 +133,46 @@ private void healthCheckInternal()
if (activeSession == null)
{
logger.info("No local CQL session is available. Cassandra is down presumably.");
- isUp = false;
+ status = null;
return;
}
maybeRegisterHostListener(activeSession);
try
{
- String version = activeSession.execute("select release_version from system.local")
- .one()
- .getString("release_version");
- isUp = true;
+ Row oneResult = activeSession.execute("select release_version, partitioner from system.local")
+ .one();
+ NodeSettings nodeSettings = new NodeSettings(oneResult.getString("release_version"),
+ oneResult.getString("partitioner"));
+ this.status = nodeSettings;
// this might swap the adapter out
- SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(version);
+ SimpleCassandraVersion newVersion = SimpleCassandraVersion.create(nodeSettings.releaseVersion());
if (!newVersion.equals(currentVersion))
{
currentVersion = newVersion;
- adapter = versionProvider.getCassandra(version).create(cqlSession);
+ adapter = versionProvider.getCassandra(nodeSettings.releaseVersion()).create(cqlSession);
logger.info("Cassandra version change detected. New adapter loaded: {}", adapter);
}
- logger.debug("Cassandra version {}", version);
+ logger.debug("Cassandra version {}", nodeSettings.releaseVersion());
}
catch (NoHostAvailableException e)
{
logger.error("Unexpected error connecting to Cassandra instance.", e);
// The cassandra node is down.
// Unregister the host listener and nullify the session in order to get a new object.
- isUp = false;
+ status = null;
maybeUnregisterHostListener(activeSession);
session = null;
Review Comment:
At this line existing session is assigned with null (throwing away the existing session). Thought it is better to close before assigning it to null.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] jyothsnakonisa commented on pull request #42: CASSANDRASC-48: Adding new endpoint that gives information about the release version & partitioner name of a node
Posted by GitBox <gi...@apache.org>.
jyothsnakonisa commented on PR #42:
URL: https://github.com/apache/cassandra-sidecar/pull/42#issuecomment-1319384194
@dineshjoshi addressed @nvharikrishna's comment on session closing.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org