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

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

tengu-alt
Copy link

@tengu-alt tengu-alt commented Jun 4, 2024

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.

@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
Author

tengu-alt commented Oct 31, 2024

@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

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
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.

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

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
Author

Choose a reason for hiding this comment

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

Agree, done.

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
Author

@tengu-alt tengu-alt Nov 7, 2024

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

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
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.

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 for CASSGO-7
@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
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.

4 participants