integration: direct node connectivity test #1189
Conversation
The following helpers are all usable universally, not only for tablets: - SingleTargetLBP, - send_statement_everywhere, - send_unprepared_query_everywhere, so it makes sense to extract them to utils.rs.
The following helpers were renamed to better convey their semantics: - `send_statement_everywhere` -> `execute_prepared_statement_everywhere`, - `send_unprepared_query_everywhere` -> `execute_unprepared_statement_everywhere`.
…y values `execute_unprepared_statement_everywhere` was limited to empty values; now any value list is supported.
`execute_(un)prepared_statement_everywhere` both have similar logic that can be extracted to a generic function. `for_each_target_execute` extraction will be even more important in the next commit, where we make its logic a bit more complex not to `unwrap()` the Sharder.
Before, `for_each_target_execute` would panic on absence of Sharder for a Node. This could be caused by two main reasons: 1) the node was unsharded (e.g., a Cassandra node), 2) there were no open connections to the node, e.g. due to the node being down. In both cases, we prefer not to panic in `for_each_target_execute` but rather assume the node is unsharded and proceed. In the case 1), execution of the request should succeed, whereas in 2) we should get the ConnectionPoolError.
I noticed that we lacked a test that requires direct connectivity to all nodes in the cluster in order to pass. Other tests would often succeed even if only one node (the initial contact point) was directly reachable. An example of a test which has exercised direct connectivity to all nodes is tablets.rs, yet it could be enabled only for non-cloud case, as it uses the proxy.
wprzytula
added
the
area/testing
|
|
|
|
||
| // I expect Scylla to not send feedback for unprepared queries, | ||
| // as such queries cannot be token-aware anyway | ||
| send_unprepared_query_everywhere( | ||
| execute_unprepared_statement_everywhere( | ||
| &session, | ||
| session.get_cluster_data().as_ref(), | ||
| &Query::new(format!("INSERT INTO {ks}.t (a, b, c) VALUES (1, 1, 'abc')")), |
There was a problem hiding this comment.
🤔 I don't really like "execute" used with unprepared statements, because EXECUTE is a CQL command to execute prepared statements. Would "send_unprepared_statement_everywhere" be an acceptable name?
There was a problem hiding this comment.
I've been thinking that once we do the execution API refactor for 2.0, there will be only one method for all kinds of statements: execute. Do you agree? If so, then why not use it here?
There was a problem hiding this comment.
I've been thinking about it lately, and I came to the conclusion that even in new execution API we should keep separate methods for unprepared statements, prepared statements, and batches.
Why? The choice of the query type should be more concious, a single method makes this less obvious.
Also the interface would be simpler to learn: user would still have methods of the struct that accept simple types, and not traits which then user needs to research and learn what actually implements them.
What I think request execution refactor should be mostly about is enabling configuration of the request, meaning we can do session.execute(something).with_timestamp(....).paging_iter().await instead of having an exponential amount of methods.
There was a problem hiding this comment.
OTOH, the fact that the CQL protocol has some specific names for execution of prepared and unprepared statements does not imply that those names are a good fit for the names of high-level user-facing API functions. It's not intuitive at all for anyone not well-versed in the CQL protocol that "query" is related to unprepared statements and "execute" to prepared statements.
If in the future we were to have distinct names for execution of different types of statements, then I'd go for:
execute_unprepared(),execute_prepared(),execute_batch().
The names then make it perfectly clear that the action is execution and the object is a particular kind of a statement.
There was a problem hiding this comment.
I agree that the names do not need to be the same (but in that case the documentation should clearly state which CQL command it corresponds to).
The proposed names are a bit too long for my taste.
Problem
I noticed that we lacked a test that requires direct connectivity to all nodes in the cluster in order to pass. Other tests would often succeed even if only one node (the initial contact point) was directly reachable.
The issue has manifested in testing serverless cloud why working on rustls support: tests would pass even with address translation disabled...
Side note
An example of a test which has exercised direct connectivity to all nodes is
tablets.rs, yet it could be enabled only for non-cloud case, as it uses the proxy.Solution
I wrote a test that iterates though all targets (pairs
(node, shard)) and sends a request directly to them (using a load balancing policy that produces singleton query plans).Bonus
I extracted and refactored some utils from
tablets.rstoutils.rs, so that they can be used by other tests.Pre-review checklist
[ ] I have provided docstrings for the public items that I want to introduce.[ ] I have adjusted the documentation in./docs/source/.[ ] I added appropriateFixes:annotations to PR description.