session: adjust naming in internal session functions

[muzarski]

During error refactor, I found it hard to understand what which function does on a request execution path. Three different names were used:

• run_query
• execute_query
• do_query

I renamed them (and all internal structures/traits related to these functions). The _query part is very misleading. It would suggest that we want to execute a CQL QUERY request, which is not true - these functions/closures are generic and allow to execute QUERY, EXECUTE and BATCH CQL requests.

Kind of related to #713

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• [ ] I added relevant tests for new features and bug fixes.
• All commits compile, pass static checks and pass test.
• PR description sums up the changes and reasons why they should be introduced.
• [ ] I have provided docstrings for the public items that I want to introduce.
• [ ] I have adjusted the documentation in ./docs/source/.
• I added appropriate Fixes: annotations to PR description.

[github-actions]

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: a4e52f2

[Lorak-mmk]

"Request" is a good word here, but you still use "execute" which is also a CQL command. Maybe "do" / "perform" / "run" are better words than "execute"?

[muzarski]

"Request" is a good word here, but you still use "execute" which is also a CQL command. Maybe "do" / "perform" / "run" are better words than "execute"?

That was one of my doubts. IMO, to execute a request fits the best, but I agree that it can suggest the EXECUTE CQL request. What I don't like about "run" is that we then would have a run_request function which then calls run_request_with_retries (or maybe I'm just overthinking, and that's totally fine). I don't really like "perform" that much. My picks would be "run_request_[with_retries/once]", "send_request_[with_retries/once]" or "do_request_[with_retries/once]" with a little bias towards "send".

[Lorak-mmk]

I don't really like "send" because we are not just sending request - we receive the response, we perform speculative execution, retries etc.
"send" would be a good word for some method in Connection that does just that.

What I don't like about "run" is that we then would have a run_request function which then calls run_request_with_retries (or maybe I'm just overthinking, and that's totally fine).

Now we have what? run_request that calls execute_request_with_retries?
I don't really see what is the difference between "running" a request and "executing" the request . Those names don't really say which layer is responsible for what, apart from "with_retries" telling us that the inner one will use retry policy.

I'm not sure what is the proper naming here tbh. Maybe this naming difficulty is a hint that the division of responsibilities between various functions should be different?

[muzarski]

I'm not sure what is the proper naming here tbh. Maybe this naming difficulty is a hint that the division of responsibilities between various functions should be different?

Current division makes sense to me. run_request (previously run_query) takes speculative execution into account, and prepares the plan (either exclusive or shared) based on whether speculative execution applies for the statement. It also constructs the RetrySession from statement config. Then it calls (directly for standard path, or indirectly in speculative_execution::execute()) execute_request_with_retries (previously execute_query) providing it the plan and retry session. execute_request_with_retries iterates over the plan, executing the request on each target sequentially wrt. plan and retry decisions.

When it comes to naming, I'll rename all of the functions/closures to run_*. As you said, run and execute do not distinguish between the execution layers anyway.

[Lorak-mmk]

I'm not sure what is the proper naming here tbh. Maybe this naming difficulty is a hint that the division of responsibilities between various functions should be different?

Current division makes sense to me. run_request (previously run_query) takes speculative execution into account, and prepares the plan (either exclusive or shared) based on whether speculative execution applies for the statement. It also constructs the RetrySession from statement config. Then it calls (directly for standard path, or indirectly in speculative_execution::execute()) execute_request_with_retries (previously execute_query) providing it the plan and retry session. execute_request_with_retries iterates over the plan, executing the request on each target sequentially wrt. plan and retry decisions.

What do you think about utilizing "speculative fiber" terminology?
run_request_speculative_fiber does convey that speculative execution won't be utilized in the function and must be instead handled by the caller.

[muzarski]

What do you think about utilizing "speculative fiber" terminology? run_request_speculative_fiber does convey that speculative execution won't be utilized in the function and must be instead handled by the caller.

Sounds good. I like it.

[muzarski]

Flaky test_coalescing test failed:

'transport::connection::tests::test_coalescing' panicked at scylla/src/transport/connection.rs:2609:78:
called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(1184), "called `Result::unwrap()` on an `Err` value: DbError(Unprepared { statement_id: b\"\\xb7/\\x95\\xb8\\xd0\\\"\\x9dq\\xcfp]\\x1a\\x9f\\xa3e<\" }, \"Prepared query with ID b72f95b8d0229d71cf705d1a9fa3653c not found (either the query was not prepared on this host (maybe the host has been restarted?) or you have prepared too many queries and it has been evicted from the internal cache)\")", ...)

[Lorak-mmk]

Flaky test_coalescing test failed:

'transport::connection::tests::test_coalescing' panicked at scylla/src/transport/connection.rs:2609:78:
called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(1184), "called `Result::unwrap()` on an `Err` value: DbError(Unprepared { statement_id: b\"\\xb7/\\x95\\xb8\\xd0\\\"\\x9dq\\xcfp]\\x1a\\x9f\\xa3e<\" }, \"Prepared query with ID b72f95b8d0229d71cf705d1a9fa3653c not found (either the query was not prepared on this host (maybe the host has been restarted?) or you have prepared too many queries and it has been evicted from the internal cache)\")", ...)

Restarted