Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include error ID in the error response for new error format #1454

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

amorton
Copy link
Contributor

@amorton amorton commented Sep 26, 2024

This was already happening for the old errors mapped to new, the new OperationAttempt work relies on this

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@amorton amorton requested a review from a team as a code owner September 26, 2024 05:05
@amorton amorton changed the title The mapping for old errors already did this, this is relied on by the… Include error ID in the error response for new error format Sep 26, 2024
@amorton amorton force-pushed the ajm/include-error-id-in-response branch from be1bb8a to adc748c Compare September 26, 2024 06:14
Copy link
Contributor

@maheshrajamani maheshrajamani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -61,6 +61,7 @@ public CommandResult.Error apply(APIException apiException) {
errorFields.put(ErrorObjectV2Constants.Fields.FAMILY, apiException.family.name());
errorFields.put(ErrorObjectV2Constants.Fields.SCOPE, apiException.scope);
errorFields.put(ErrorObjectV2Constants.Fields.TITLE, apiException.title);
errorFields.put(ErrorObjectV2Constants.Fields.ID, apiException.errorId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java doc to describe what these fields are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a whole bunch of docs on the ApiException that explains all the fields, your comment has reminded me that it's about time we refactor the CommandResult.Error it is a mess with the map.

if we do this, then we dont need all these fields and maps etc see #1462

@amorton amorton merged commit 51492b0 into main Sep 27, 2024
3 checks passed
@amorton amorton deleted the ajm/include-error-id-in-response branch September 27, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants