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

Use WarningException, expand CommandResultBuilder #1519

Merged
merged 27 commits into from
Oct 21, 2024
Merged

Conversation

amorton
Copy link
Contributor

@amorton amorton commented Oct 9, 2024

Fixes #1497

Changes the warnings associated commands to use the new ApiExcpetions,
and warnings in the return status now returns a list of error object V2.

To do that needed to improve the way CommandResult was built, so it always
had a status map so the CommandProcessor could append a warning. To do that
expanded the CommandResultBuilder added for the OperationAttempts, removed
the many overloads used for CommandResult, and updated all creation of the
CommandResult to use the builder.

See also #1518 to continue 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

Changes the warnings associated commands to use the new ApiExcpetions,
and `warnings` in the return status now returns a list of error object V2.

To do that needed to improve the way CommandResult was built, so it always
had a status map so the CommandProcessor could append a warning. To do that
expanded the CommandResultBuilder added for the OperationAttempts, removed
the many overloads used for CommandResult, and updated all creation of the
CommandResult to use the builder.

See also #1518 to continue this
@amorton amorton requested a review from a team as a code owner October 9, 2024 03:00
…cResponses

Missed adding errors to the CommandResult builder for
per document responses in InsertOperationPage
@tatu-at-datastax
Copy link
Contributor

I have a philosophical objection here: Exceptions should be used for Exceptional situations, failures, and not as internal communication mechanism (i.e. not for convenience). And warnings by their nature are not such things.
It seems to me usage here is due to convenience of using message construction/templating part, and not because Exceptions would make sense otherwise.

Beyond philosophical part, Exceptions are also rather expensive things to use wrt performance: they are inefficient things to use if (although only if) created fresh, with stack traces.

So I am not sure we should take this approach for generating warnings as part of response payload.

- change CommandErrorV2 property to match bean style
- change CreateKeyspaceIntegrationTest to get new response warning
@amorton
Copy link
Contributor Author

amorton commented Oct 15, 2024

Beyond philosophical part, Exceptions are also rather expensive things to use wrt performance: they are inefficient things to use if (although only if) created fresh, with stack traces.

I can make the WarningException overirde fillInStackTrace() so it does not create a stack trace, and I can see a way to run the template without needing to instantiate a an ApiException sub class, I'll spin up a ticket when this is ready. It is already almost there: running the template creates a ErrorInstance record, which is used to construct the ApiException, we can use that and just then need a way to turn them into the CommandErrorV2 for serialisation. Should be a reasonable fast follow with no external impact .

It seems to me usage here is due to convenience of using message construction/templating part, and not because Exceptions would make sense otherwise.

Yup I was optimising for managing the content in the yaml file, and having a standard structure for errors and warnings.

and made both create and drop tests check the names of the commands
in the message
@amorton amorton mentioned this pull request Oct 15, 2024
4 tasks
- our schema cache was not invalidating when a table was changed, so
we would always give out missing index errors rather than errors when
index was there but did not support the operation

- updated integration tests for the full error in the warning, and
made them check the id for the warning

- big fix, we were not turning on allow filtering when doing a
comparison query for some data types on indexed columns
@amorton
Copy link
Contributor Author

amorton commented Oct 16, 2024

@tatu-at-datastax created this to track the change to not use a full exception #1549

there is a bug where the data element was not returned when
there were not document, these were not found by exiting tests

tests updated to check the top three fields in the response are present
as expected see ResponseAssertions
failing tests in placed for data element not been added
in the command result
@amorton amorton merged commit fba4b13 into main Oct 21, 2024
3 checks passed
@amorton amorton deleted the ajm/warnings-full-error branch October 21, 2024 19:20
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.

Change the warnings section of the status to be full error object
3 participants