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.

Sign up for GitHub

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

Jump to bottom

Refactor to add OperationAttempt #1434

Merged
merged 27 commits into from
Oct 1, 2024
Merged

Refactor to add OperationAttempt #1434

merged 27 commits into from
Oct 1, 2024

Conversation

amorton
Copy link
Contributor

@amorton amorton commented Sep 19, 2024 •
edited
Loading

Build a general OperationAttempt and a GenericOperation that can process any type of operation attempt.

Will eventually lead to a single Operation implementation, removing a lot of duplication between the different Operation classes in how they interact with the DB.

Guide:

  • most of the heavy lifting is in io.stargate.sgv2.jsonapi.service.operation where it defines the OperationAttempt and where uses it in GenericOperation
  • in io.stargate.sgv2.jsonapi.service.operation.tables; the tables code uses it, this has not been rolled out into Delete and Update operations yet. And no work has been done for Collections
  • no tests on the OperationAttempt code yet, but the tests for the parts the use it work. I am working on unit tests, but we want to get this code into main so others can use it.

Which issue(s) this PR fixes:
Fixes #1424

Checklist

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

@amorton
Start refactoring CollectionSchemaObject
a22b355 
for #1424, will need tablemetadta on the collectionschema object

it was a giant mess, still is, but trying to cleanup gradually
@amorton
WIP refactoring to add OperationAttempt
04031fb 
runs, not all unit tests checked

Adds OperationAttempt as a base "task" that any operation runs,
example in InsertTableOperation and InsertManyCommandResolver

No tests written yet.
@amorton amorton changed the base branch from main to ajm/start-collection-refactor September 19, 2024 02:58
@Yuqi-Du Yuqi-Du mentioned this pull request Sep 23, 2024
Closed
@amorton
adding comments and code tidy
a17d2f3
@amorton
trying to get the response for insertId's correct
f643396 
again
@amorton
trying to get insertedId's correct again
bcc3a9c
@amorton
Test fixes
5cedeba
@amorton
fmt
5812835
@amorton
comments and cleanup
93a1369
@amorton
Merge branch 'main' into ajm/start-collection-refactor
cf249c3
@amorton
Merge branch 'ajm/start-collection-refactor' into ajm/#1424-refactor-…
b347d40 
…TableInsertAttempt
@amorton
Add CommandResultBuilder
71e20fb 
Because there are several ways we create this, the builder is used in
the work for OperationAttempt
@amorton
Include errorID in response
5cb12bd
@amorton
remove log that should not be there
7ad4be0
@amorton
move RequestContext into CommandQueryExecutor
260282b 
that is where is is used
@amorton
comments and tidy
482b44a
@amorton
fix: SchemaAttempt was calling wrong execute
b4e777a 
added driver profile
reset logging to Debug
@amorton
compile fix
5b8bea7
@amorton amorton marked this pull request as ready for review September 26, 2024 06:57
@amorton amorton requested a review from a team as a code owner September 26, 2024 06:57
@amorton amorton changed the title Refactor to add OperationAttempt fixes #1424 Refactor to add OperationAttempt Sep 26, 2024
Base automatically changed from ajm/start-collection-refactor to main September 26, 2024 23:30
This was referenced Sep 27, 2024
Open
Open
Open
Open
tatu-at-datastax
tatu-at-datastax approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

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

To be honest, this is big enough that I couldn't dig very deep. But I trust the test coverage, plus ability to fix things post-merge.

And we need to get this merged to avoid blocking other PRs that come after wrt createTable(), codecs.

maheshrajamani
maheshrajamani approved these changes Oct 1, 2024
View reviewed changes
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.

Operation Attempt and executor looks good.
LGTM

src/main/java/io/stargate/sgv2/jsonapi/service/operation/query/CqlOptions.java Show resolved Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/query/CqlOptions.java Show resolved Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/query/CQLOption.java Show resolved Hide resolved
@amorton amorton mentioned this pull request Oct 1, 2024
Open
@amorton amorton merged commit 118d9f0 into main Oct 1, 2024
3 checks passed
@amorton amorton deleted the ajm/#1424-refactor-TableInsertAttempt branch October 1, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yuqi-Du Yuqi-Du Yuqi-Du left review comments

@tatu-at-datastax tatu-at-datastax tatu-at-datastax approved these changes

@maheshrajamani maheshrajamani maheshrajamani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

refactor table insert operation, move calling the driver to TableInsertAttempt
4 participants
@amorton @tatu-at-datastax @maheshrajamani @Yuqi-Du