IT: Support set_record_contacted_hosts() and get_attempted_hosts_from_future()
#322
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BoundStatement was manually constructed in two places. This commit extracts its internal constructor (`new()`) and uses it in two new public constructors: `new_unprepared()` and `new_prepared()`, for deduplication and readability.
We no longer use the term "query" for unprepared statements in the Rust Driver nor the wrapper. This commit renames `BoundSimpleQuery` to `BoundSimpleStatement` to reflect this change.
This is the first of the two commits that implement and expose recording hosts that were attempted during execution of a statement. This is useful for testing purposes, allowing us to verify which hosts were attempted during a request. In particular, some existing integration tests rely on this feature. Implementing this will allow us to run those tests, as well as write new tests with more assertions regarding routing. Implementation involves: - Implementing `RecordingHistoryListener` that records attempted hosts; - Adding exposed `testing_statement_set_recording_history_listener` to enable recording in the statement's history listener; - Modifying the session execution to set the history listener if recording is enabled; - Adding a method to retrieve the recorded hosts from the listener (which will be used in the next commit).
There was a problem hiding this comment.
Pull Request Overview
This PR re-implements support for recording and retrieving the hosts attempted during statement execution by adding new Rust wrapper functions and integration testing functionality.
- Implements new C API functions in the header and their corresponding behavior in the C++ and Rust code.
- Refactors simple statement naming from BoundSimpleQuery to BoundSimpleStatement and updates CassStatement creation.
- Integrates recording of attempted hosts via a new RecordingHistoryListener in both future and session handling, with test support and Makefile changes.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/testing_rust_impls.h | Adds declarations for setting a recording history listener and retrieving attempted hosts. |
| src/testing.cpp | Implements get_attempted_hosts_from_future() using a newly allocated concatenated string. |
| scylla-rust-wrapper/src/statement.rs | Refactors simple statement naming and updates the new_unprepared constructor. |
| scylla-rust-wrapper/src/session.rs | Passes the recording listener in CassFuture::make_raw and related functions. |
| scylla-rust-wrapper/src/prepared.rs | Updates CassStatement construction for prepared statements. |
| scylla-rust-wrapper/src/integration_testing.rs | Implements the recording listener, associated C API functions, and test case. |
| scylla-rust-wrapper/src/future.rs | Introduces a recording_listener field and attempted_hosts() method in CassFuture. |
| Makefile | Updates the test command with new configuration flags. |
| .github/pull_request_template.md | Adds a checklist item for Fixes annotations. |
Lorak-mmk
requested changes
Jun 17, 2025
Comment on lines
294
to
+295
| run-test-unit: install-cargo-if-missing _update-rust-tooling | ||
| @cd ${CURRENT_DIR}/scylla-rust-wrapper; cargo test | ||
| @cd ${CURRENT_DIR}/scylla-rust-wrapper; RUSTFLAGS="--cfg cpp_rust_unstable --cfg cpp_integration_testing" cargo test |
There was a problem hiding this comment.
Unrelated: I find this style of Makefile very weird. One reason is that it assumes users env (in this case it uses apt). Other is that is performs unrelated actions (installing cargo using a package manager) when I told it to run tests.
Its fine to have targets / scripts that install stuff, but they should not be in dependencies of normal targets. If they are needed by CI, they should be explicitly called by CI.
There was a problem hiding this comment.
You're more than welcome to improve this Makefile!
Especially if this makes it run on my Fedora, or Arch.
wprzytula
force-pushed
the
support-set_record_contacted_hosts
branch
from
4eca162 to
ad90c81
Compare
wprzytula
force-pushed
the
support-set_record_contacted_hosts
branch
from
ad90c81 to
68edd1d
Compare
Lorak-mmk
approved these changes
Jun 17, 2025
wprzytula
force-pushed
the
support-set_record_contacted_hosts
branch
from
68edd1d to
5ed14a4
Compare
This commit exposes the `attempted_hosts` method on `CassFuture`, allowing users to retrieve the hosts that were attempted during the execution of a future. This can be used in proxy tests in the wrapper itself, but the main use case is in the C++ integration tests. The necessary bridging code is implemented in the next commit.
This commit implements the function `testing_future_get_attempted_hosts` in Rust, as well as includes a test for the new function. It's going to be used in the C++ integration tests. I had tough time figuring out how to return a list of attempted hosts' addresses to C code, so I decided to return a concatenated string of them in a stringified form, delimited by '\n'. The overhead this introduces is not important at all, as these are just integration tests. The caller is responsible for freeing the memory. They must do so by calling `testing_free_cstring`, because the string is allocated in Rust. The new unit test, unlike all unit tests so far, is put behind the #[cfg(cpp_integration_testing)] attribute. In order to run it, we must to supply the `cpp_integration_testing` flag to RUSTFLAGS. So far, RUSTFLAGS were configured in .cargo/config.toml, in `[build]`. As this method of setting RUSTFLAGS does not allow setting different flags for test target and for the rest of the build, I decided to set the necessary RUSTFLAGS for tests in the Makefile. Keep in mind that this will require us to keep the flags there and in the `.cargo/config.toml` in sync, unfortunately.
`get_attempted_hosts_from_future` was originally implemented in C++ using the internal C++ Driver API. This commit reimplements it using the new exposed Rust API, namely `testing_future_get_attempted_hosts`. To finish up, the commit also reimplements `set_record_attempted_hosts`, which is tightly coupled with the previous function, using `testing_statement_set_recording_history_listener` (introduced a couple of commits ago). This completes the re-implementation of this testing functionality in Rust. I'd like to enable some tests that depend on this functionality, but none of them has all the requirements met yet. Once I rework the DCAware LBP in a follow-up, I'll be able to enable its tests thanks to this commit.
wprzytula
force-pushed
the
support-set_record_contacted_hosts
branch
from
5ed14a4 to
dbda206
Compare
Lorak-mmk
approved these changes
Jun 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR re-implements
set_record_contacted_hosts()and associatedget_attempted_hosts_from_future()integration test functions in the Rust wrapper.For now, no test using it is enabled. This is because none of them has its other requirements met. In #321, I'll be able to enable DCAware test suite.
Pre-review checklist
[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.