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

Allow implementing ExecutableQuery interface #1759

Open
worryg0d opened this issue May 31, 2024 · 9 comments
Open

Allow implementing ExecutableQuery interface #1759

worryg0d opened this issue May 31, 2024 · 9 comments

Comments

@worryg0d
Copy link

worryg0d commented May 31, 2024

The driver allows users to provide their implementations of HostSelectionPolicy. However, it contains Pick() method that requires an arg of the ExecutableQuery type. The ExecutableQuery interface can't be implemented outside the pkg because it consists of private methods such as execute(), attempt(), etc.
The driver should allow users to implement ExecutableQuery to allow them to test their custom implementations of anything that requires ExecutableQuery.

@joao-r-reis
Copy link
Contributor

Ensure host selection policies can be implemented outside the package.

Why can't these policies be implemented outside the package at the moment?

@worryg0d
Copy link
Author

As far as I remember they actually can, but we can't properly test them because there is Pick() method that requires an ExecutableQuery that can't be mocked because it consists of some private methods such as execute, attempt

@joao-r-reis
Copy link
Contributor

Ok then we should change the title and description of the issue

@worryg0d worryg0d changed the title Allow Implementation of Host Selection Policy Outside the Package Allow implementing ExecutableQuery interface Nov 25, 2024
@worryg0d
Copy link
Author

Yes, it makes sense since it touches the implementation side, but it is misleading in understanding the problem. I changed the title and described in detail what the actual problem is.

@joao-r-reis
Copy link
Contributor

Hmm with this title I don't think it's related to #1754 anymore is it?

@worryg0d
Copy link
Author

I think so, it can be closed for now...

@martin-sucha
Copy link
Contributor

There are also other options how to achieve testability of HostSelectionPolicy implementations outside of the gocql package.

If this gets changed in a major version, the HostSelectionPolicy.Pick could take another type instead of ExecutableQuery. Which information is actually useful for the HostSelectionPolicy implementation? For example SetConsistency(c Consistency) isn't necessary, while Attempts() is racy.

Also, passing a struct instead of interface would allow for new fields to be added in backwards compatible way without the users needing to type-check everything. By the way, is it possible to type cast the ExecutableQuery to *Query or *Batch now in the HostSelectionPolicy implementation?

Another option would be to provide a mock ExecutableQuery implementation directly in the gocql package.

@worryg0d
Copy link
Author

passing a struct instead of interface would allow for new fields to be added in backwards compatible way without the users needing to type-check everything

It sounds like a good idea to me. It will allow us to append new useful info (if any) without any breaking changes. Also, it will enable users to easily test their implementations of HostSelectionPolicy without needing to mock the ExecutableQuery.

is it possible to type cast the ExecutableQuery to *Query or *Batch now in the HostSelectionPolicy implementation?

On the current codebase yes, it is.

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 28, 2024

By the way, is it possible to type cast the ExecutableQuery to *Query or *Batch now in the HostSelectionPolicy implementation?

This is also an issue for CASSGO-22, the current proposal (comment here) is to add a RetryableQuery interface (implemented by a new internal struct that is not immutable so we can make Query/Batch immutable) and add a GetQuery() method to RetryableQuery so get the original Query or Batch object if a custom retry policy needs to access it.

@martin-sucha can you provide feedback on that ticket (CASSGO-22)? It's potentially a significant change of the API so it would be nice to get as much feedback as possible. CASSGO-22 changes will impact what happens with this current issue as well. It would be great to improve this part of the API before the 2.0 release.

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 a pull request may close this issue.

3 participants