CASSGO-92: Add a public method to retrieve StatementMetadata and LogField methods #1915
Conversation
| s.pool = cfg.PoolConfig.buildPool(s) | ||
| s.policy = cfg.PoolConfig.HostSelectionPolicy | ||
|
|
||
| // set the executor here in case the policy needs to execute queries in Init |
There was a problem hiding this comment.
Allowing queries to be executed before the session is even initialized is so confusing to me, the session doesn't even have a control connection until it is initialized. I understand the motivation behind letting policies query something in the DB to initialize but I feel like if such use case does exist it should be better to add a new method to the policy called OnSessionInit or something that will be called by the session after its initialization has completed. However adding a new method to the interface is a breaking change so we'd have to add a new interface and type check if the policy implements the new interface
There was a problem hiding this comment.
I feel like this use case is so niche that it would be better to tell those users to implement a policy with "two states" and the user is responsible for changing the state of that policy when they create a session
There was a problem hiding this comment.
Its not obvious right now because it's called Init and it's passed a session but it doesn't say the session isn't ready or isn't usable. If the policy is ready there isn't a reason why the policy should have assumed the session isn't.
I'm not arguing that we change things but we should at least document the intention of init and the state of the session.
There was a problem hiding this comment.
yeah honestly it doesn't make a lot of sense that we're passing the Session in the Init method because it's not clear exactly which methods of the Session type API can actually be used by the policy at that point. I think the best we can do at this point is to document this in the the Init method docs because any other way of addressing this probably requires a breaking change
There was a problem hiding this comment.
I updated the interface to clarify Init. That said, I want to keep the changes I made since they're small and prevent the panic. In a host policy I was debugging, it starts a background goroutine to refresh the partitions and if that runs too soon it could still cause a panic without these changes.
There was a problem hiding this comment.
👍 I'm actually curious to know what happens now with these changes, there is an executor but there is no hosts or connection pools, the ideal fix would be for the policy to get notified somehow that the session has finished initializing. Ideally the driver would do that on its own but without this I guess the user creating the policy has to write that code on the app side
In any case we can keep these changes sure
I wasn't necessarily intending on it being realistic to fix all of the things I brought up especially the ones with workarounds. Could I just make another issue for the |
That makes sense, I'd say we rename the issue to
Yeah go for it. I'm also not sure about whether it's worth breaking the interface, I think I'd rather document how users can achieve that by type casting the object returned by I do think we should create JIRAs for these things that require breaking changes and tag them with an appropriate label so that we can address them when we work on a 3.0. I'd say creating one to review the HostSelectionPolicy API (especially the Init method) and one to review |
jameshartig
changed the title
jameshartig
force-pushed
the
x-cassgo-92
branch
from
9d70df4 to
6129d3d
Compare
|
@joao-r-reis updated the JIRA issue and updated this PR according to the comments. |
…ld methods StatementMetadata can be called on a session to get the bind, result, and pk information for a given query. Previously this wasn't publicly exposed but is necessary for some implementations of HostSelectionPolicy like token-aware. This might also be useful for CI tooling or runtime analysis of queries and the types of columns. NewLogField* are methods to to return a LogField with name and a specific type. Finally, session init was cleaned up to prevent a HostSelectionPolicy from causing a panic if it tried to make a query during init. The interface was documented that queries should not be attempted. Patch by James Hartig for CASSGO-92; reviewed by João Reis for CASSGO-92
jameshartig
force-pushed
the
x-cassgo-92
branch
from
6129d3d to
291756b
Compare
2 participants
StatementMetadata can be called on a session to get the bind, result, and pk information for a given query. Previously this wasn't publicly exposed but is necessary for some implementations of HostSelectionPolicy like token-aware. This might also be useful for CI tooling or runtime analysis of queries and the types of columns.
NewLogField* are methods to to return a LogField with name and a specific type.
Finally, session init was cleaned up to prevent a HostSelectionPolicy from causing a panic if it tried to make a query during init. The interface was documented that queries should not be attempted.
Patch by James Hartig for CASSGO-92