-
Notifications
You must be signed in to change notification settings - Fork 24
Batch #13 of ErrorCodeV1 removal (see #2285) #2323
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
Conversation
📉 Unit Test Coverage Delta vs Main Branch
|
Unit Test Coverage Report
|
| // Support for converting legacy ErrorCodeV1 usage | ||
| // Copied from JsonApiException with modifications | ||
| @Override | ||
| public CommandResult get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed because of the way Mutiny pipeline is defined in CreateCollectionOperation (and probably in other places); instead of exception type, Supplier<CommandResult> is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 on this, lets discuss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will revert, as discussed.
📈 Integration Test Coverage Delta vs Main Branch (dse69-it)
|
Integration Test Coverage Report (dse69-it)
|
📈 Integration Test Coverage Delta vs Main Branch (hcd-it)
|
Integration Test Coverage Report (hcd-it)
|
|
|
||
| // CreateCollection error codes: | ||
|
|
||
| EMBEDDING_SERVICE_NOT_CONFIGURED( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved next to related codes; to be converted in follow-up PR.
| Underlying problem: ${errorMessage} | ||
| Report the problem to database administrators. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were moved to alphabetic position, not changed in any way.
| ${SNIPPET.RETRY} | ||
| - scope: DATABASE | ||
| code: INVALID_COLLECTION_QUERY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed from INVALID_QUERY, but usage should probably be eventually unified with INVALID_DATABASE_QUERY as part of query handling changes.
| // CreateCollection error codes: | ||
|
|
||
| EXISTING_TABLE_NOT_DATA_API_COLLECTION("Existing table is not a valid Data API collection"), | ||
| COLLECTION_CREATION_ERROR( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot yet convert this one due to the way it is returned (not throw) in CreateCollectionOperation, handle after @amorton's refactoring of DB access.
| CANNOT_RENAME_UNKNOWN_TYPE_FIELD, | ||
| CANNOT_VECTORIZE_NON_VECTOR_COLUMNS, | ||
| CANNOT_VECTORIZE_UNKNOWN_COLUMNS, | ||
| COLLECTION_EXISTS_WITH_DIFFERENT_SETTINGS, // converted from ErrorCodeV1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming undone: changed back to EXISTING_COLLECTION_DIFFERENT_SETTINGS for compatibility reasons.
| - scope: SCHEMA | ||
| code: COLLECTION_NOT_EXIST | ||
| title: Collection already exists with different settings | ||
| title: Collection or table does not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed an earlier copy-paste problem.
0d13d9a to
e7bfeba
Compare
…emoval-13' into tatu/2285-errorv1-removal-13
What this PR does:
Converts more
ErrorCodeV1entries into V2 typed exceptionsWhich issue(s) this PR fixes:
Part of #2285
Checklist