yifan-c commented on code in PR #366:
URL: https://github.com/apache/cassandra-sidecar/pull/366#discussion_r3448026677
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/RingHandlerIntegrationTest.java:
##########
@@ -69,8 +69,8 @@ void retrieveRingWithUnknownKeyspace(VertxTestContext
context) throws Exception
assertThat(response.statusCode()).isEqualTo(errorCode);
JsonObject error = response.bodyAsJsonObject();
assertThat(error.getInteger("code")).isEqualTo(errorCode);
- assertThat(error.getString("message")).contains("The keyspace
unknown_ks, does not exist");
-
+ assertThat(error.getString("message"))
+ .contains("The requested keyspace Name{unquotedName='unknown_ks',
maybeQuotedName='unknown_ks'} was not found.");
Review Comment:
How is the new error message better? It also seems to be unrelated to the
goal – "Remove filesytem path from Http response"
##########
server/src/main/java/org/apache/cassandra/sidecar/handlers/snapshots/ListSnapshotHandler.java:
##########
@@ -154,7 +154,7 @@ protected void processFailure(Throwable cause,
request, remoteAddress, host, context.request().method(),
cause);
if (cause instanceof FileNotFoundException || cause instanceof
NoSuchFileException)
{
- context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND,
cause.getMessage()));
+ context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND, "The
requested snapshot folder was not found. " + request));
Review Comment:
Suggestion:
1. Update message to "The requested snapshot was not found"
2. Remove `request: SnapshotRequestParam` in the message, as the caller
already have this context.
##########
server/src/main/java/org/apache/cassandra/sidecar/handlers/snapshots/ClearSnapshotHandler.java:
##########
@@ -100,7 +100,7 @@ protected void processFailure(Throwable cause,
requestParams, remoteAddress, host,
context.request().method(), cause);
if (cause instanceof FileNotFoundException || cause instanceof
NoSuchFileException)
{
- context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND,
cause.getMessage()));
+ context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND,
"Snapshot could not be cleared, directory not found. " + requestParams));
Review Comment:
Suggestion:
1. Update message to "The requested snapshot was not found"
2. Remove `requestParams: SnapshotRequestParam` as the caller already have
this context.
##########
server/src/main/java/org/apache/cassandra/sidecar/handlers/sstableuploads/SSTableImportHandler.java:
##########
@@ -149,7 +149,7 @@ protected void processFailure(Throwable cause,
{
logger.error("Upload directory not found for request={},
remoteAddress={}, " +
"instance={}", request, remoteAddress, host, cause);
- context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND,
cause.getMessage()));
+ context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND,
"SSTable could not be uploaded to desired upload location. " + request));
Review Comment:
I agree to Shailaja's suggestion. The updated error message "could not be
uploaded" suggests a server error, but it is indeed 404, a client error. We
should craft the message to reflect the error directly. My suggestion: "SSTable
upload directory was not found".
On adding the `request: SSTableImportRequestParam` to the error message. I
do not believe including the options like "resetLevel" in the error message
provides value. Let's remove it.
--
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]
