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

CASSGO-7 Change Batch API to be consistent with Query() #1764

Merged
merged 1 commit into from
Nov 25, 2024
Merged

CASSGO-7 Change Batch API to be consistent with Query() #1764

merged 1 commit into from
Nov 25, 2024
+63 −34

Conversation

tengu-alt
Copy link
Contributor

@tengu-alt tengu-alt commented Jun 4, 2024 •
edited
Loading

According to issue #1187, I have refactored the Query() method for the batch and implemented the Exec() method for the batch structure to save the general pattern of the query execution, and saved the older method (session.ExecuteBatch()) for backward compatibility.
This means the batch for now behaves the same way as query.

@tengu-alt tengu-alt force-pushed the batch-exec branch 3 times, most recently from 5a44475 to a6d1a09 Compare October 14, 2024 09:01
@joao-r-reis
Copy link
Contributor

@tengu-alt we should also add a new Batch() function and deprecate the existing NewBatch (basically a "soft" rename of NewBatch() -> Batch() without breaking users) and deprecate Session.ExecuteBatch.

NewBatch and ExecuteBatch can then be removed in 3.0 so that users have more than enough time to transition to the new names.

@joao-r-reis joao-r-reis changed the title Exec() method for batch was added & Query() method was refactored CASSGO-7 Exec() method for batch was added & Query() method was refactored Oct 29, 2024
@tengu-alt
Copy link
Contributor Author

tengu-alt commented Oct 31, 2024 •
edited
Loading

@tengu-alt we should also add a new Batch() function and deprecate the existing NewBatch (basically a "soft" rename of NewBatch() -> Batch() without breaking users) and deprecate Session.ExecuteBatch.

NewBatch and ExecuteBatch can then be removed in 3.0 so that users have more than enough time to transition to the new names.

@joao-r-reis I do not clearly understand. The NewBatch() function is already marked as deprecated and session.NewBatch is recommended to use instead. Also I can't name the function same as struct (Batch) because the Batch is already declared.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Oct 31, 2024 •
edited
Loading

Session.NewBatch() should be deprecated now, global NewBatch() can be removed since it was deprecated already and Session.Batch() should be added to replace Session.NewBatch() (keep the same functionality).

Session.Batch() does not have a conflict with the type Batch, you can have both (in fact this already exists with Query())

The original issue mentions the rename Session.NewBatch() to Session.Batch() already, my only additional suggestion here is to keep and deprecate Session.NewBatch() for now AND add the new Session.Batch().

@joao-r-reis
Copy link
Contributor

global NewBatch() removal is already handled by #1762 so it doesn't have to happen here

@tengu-alt
Copy link
Contributor Author

Session.NewBatch() should be deprecated now, global NewBatch() can be removed since it was deprecated already and Session.Batch() should be added to replace Session.NewBatch() (keep the same functionality).

Session.Batch() does not have a conflict with the type Batch, you can have both (in fact this already exists with Query())

The original issue mentions the rename Session.NewBatch() to Session.Batch() already, my only additional suggestion here is to keep and deprecate Session.NewBatch() for now AND add the new Session.Batch().

I see, thank you. Done, also I've changed the Session.NewBatch() usage in .doc and tests.

worryg0d
worryg0d reviewed Nov 5, 2024
View reviewed changes
session.go
Comment on lines +734 to +739
func (b *Batch) Exec() error {
iter := b.session.executeBatch(b)
return iter.Close()
}
Copy link

@worryg0d worryg0d Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you should add a doc here because it is meant to be used by users. Maybe use the same description as for Session.ExecuteBatch().

Also, add a doc for Session.Batch() too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, done.

joao-r-reis
joao-r-reis reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@joao-r-reis joao-r-reis left a comment

Choose a reason for hiding this comment

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

LGTM, just 1 comment about the duplicate code in the new function

session.go Outdated
spec: &NonSpeculativeExecution{},
routingInfo: &queryRoutingInfo{},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate code (NewBatch)

Copy link
Contributor Author

@tengu-alt tengu-alt Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I kept the session.NewBatch and marked it as deprecated according to your suggestion.

Copy link
Contributor

@joao-r-reis joao-r-reis Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes thanks but please create a private function that both can call or make one of them call the other so we don't have the duplicate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Fixed.

@joao-r-reis
Copy link
Contributor

Please format your commit message following the rules stated in CONTRIBUTING.md and add this JIRA to the CHANGELOG.md in the Changed section of Unreleased, thanks!

We need another committer +1 to merge this.

@tengu-alt tengu-alt changed the title CASSGO-7 Exec() method for batch was added & Query() method was refactored CASSGO-7 Change Batch API to be consistent with Query() Nov 18, 2024
@joao-r-reis
Copy link
Contributor

Sorry I merged another PR which is causing a git conflict on CHANGELOG.md can you fix it and I'll merge this right after? Otherwise I can merge manually and fix it in place

@tengu-alt
Change Batch API to be consistent with Query()
9007bd0 
Exec() method for batch was added & Query() method was refactored.
Batch for now behaves the same way as query.

patch by Oleksandr Luzhniy; reviewed by João Reis, Danylo Savchenko, Bohdan Siryk, Jackson Fleming, for CASSGO-7
@tengu-alt
Copy link
Contributor Author

tengu-alt commented Nov 23, 2024 •
edited
Loading

Sorry I merged another PR which is causing a git conflict on CHANGELOG.md can you fix it and I'll merge this right after? Otherwise I can merge manually and fix it in place

Fixed! Thank you for the hint.

@joao-r-reis joao-r-reis merged commit 48bb2bc into apache:trunk Nov 25, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worryg0d worryg0d worryg0d left review comments

@testisnullus testisnullus testisnullus approved these changes

@joao-r-reis joao-r-reis joao-r-reis approved these changes

@Jacko161 Jacko161 Jacko161 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.
5 participants
@tengu-alt @joao-r-reis @Jacko161 @testisnullus @worryg0d