CASSSIDECAR-477: Remove filesytem path from Http response#366
Conversation
| 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)); |
There was a problem hiding this comment.
[minor] message can be in similar lines to above logger.error message - '"Upload directory not found ...'
There was a problem hiding this 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.
| 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."); |
There was a problem hiding this comment.
How is the new error message better? It also seems to be unrelated to the goal – "Remove filesytem path from Http response"
| 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)); |
There was a problem hiding this 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.
| 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)); |
There was a problem hiding this comment.
Suggestion:
- Update message to "The requested snapshot was not found"
- Remove
request: SnapshotRequestParamin the message, as the caller already have this context.
| 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)); |
There was a problem hiding this comment.
Suggestion:
- Update message to "The requested snapshot was not found"
- Remove
requestParams: SnapshotRequestParamas the caller already have this context.
5 participants
No description provided.