Adjust and enable some ControlConnection integration tests
#255
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
muzarski
added
CI
Lorak-mmk
approved these changes
Apr 16, 2025
There was a problem hiding this comment.
Note: As we can see, rust-driver does not provide the node address
either in the log message or in the error type.
This is something we should consider adding to rust-driver.
Please open an issue about it
- TopologyChange <- some error during
ccm addcommand (need to investigate)
In a follow up or before merging this?
Ok.
I'll open separate issue regarding this. I'm not sure how much effort this would require - is it just a fix on cpp-rust-driver side? Or maybe there is some bug in ccm. Who knows... |
At the time of writing this, I confused the address translation with DNS address resolution. In addition, the `CASS_ERROR_LIB_HOST_RESOLUTION` in cpp-driver is only returned from `cass_cluster_set_local_address()` when unparsable ip address is provided.
Motivation (based on cpp-driver):
1. UNABLE_TO_CONNECT should only be returned when user tries to call
`cass_session_connect` on a session that is already connecting, connected or closing.
We already handle this case in `session.rs`.
```
let mut session_guard = session_opt.write().await;
if session_guard.is_some() {
return Err((
CassError::CASS_ERROR_LIB_UNABLE_TO_CONNECT,
"Already connecting, closing, or connected".msg(),
));
}
```
2. NO_HOSTS_AVAILABLE is returned in all places where driver cannot reach
one of the nodes. This includes:
- pool is broken
- connection is broken
- connection is closed
- connection is refused
- empty LB plan
- user provided empty list of contact points
... etc.
Thanks to this change, we will also be able to enable new integration tests
that expect `NO_HOSTS_AVAILABLE` error.
As a side note: as you can see, the NO_HOST_AVAILABLE is mapped from a lot of
Rust error types/variants. I believe we should increase the granularity of
CassError enum and add more error variants. This way we will be able to
define more direct mapping between Rust and cpp-rust-driver errors.
muzarski
force-pushed
the
control-connection-it
branch
from
96463e6 to
b39fa51
Compare
|
Rebased on |
Lorak-mmk
approved these changes
Apr 16, 2025
wprzytula
requested changes
Apr 19, 2025
Comment on lines
239
to
241
| logger_.add_critera("Could not fetch metadata, error: " | ||
| "Control connection pool error: The pool is broken; " | ||
| "Last connection failed with: Invalid argument (os error 22)"); |
There was a problem hiding this comment.
BTW, do we target Windows? If so, then we should run unit & integration tests in the CI on Windows, too.
There was a problem hiding this comment.
I believe we want to support it eventually: #130
muzarski
force-pushed
the
control-connection-it
branch
from
b39fa51 to
92e2f7c
Compare
|
v1.1: Addressed @wprzytula comments. |
wprzytula
requested changes
Apr 19, 2025
I tried to map the underlying errors as precisely as possible (as of now). The most important one is the `MetadataError::ConnectionPoolError` which should be mapped to `LIB_NO_HOSTS_AVAILABLE`. This is required by the test.
We decided that `cass_cluster_set_local_address()` should return LIB_BAD_PARAMS in case user provides unparsable ip address.
Note: As we can see, rust-driver does not provide the node address either in the log message or in the error type. This is something we should consider adding to rust-driver.
Tests enabled: - ConnectUsingInvalidIpAddress - ConnectUsingInvalidPort - ConnectUsingUnresolvableLocalIpAddress - ConnectUsingUnbindableLocalIpAddress - ConnectUsingValidLocalIpAddressButInvalidRemote Tests that we keep disabled: - TopologyChange <- some error during `ccm add` command (need to investigate) - FullOutage <- requires cass_cluster_set_constant_reconnect - TerminatedUsingMultipleIoThreadsWithError <- requires cass_cluster_set_num_threads_io
muzarski
force-pushed
the
control-connection-it
branch
from
92e2f7c to
5baa5d3
Compare
wprzytula
approved these changes
Apr 19, 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.
Ref: #132
Depends on: #254. Start review from
cass_error: remove misleading comment.To enable the tests I firstly needed to do some adjustments:
UNABLE_TO_CONNECTerror in places whereLIB_NO_HOSTS_AVAILABLEshould be returned. This is adjusted.cass_cluster_set_local_addressto returnLIB_HOST_RESOLUTIONwhen unparsable ip address is provided. We decided that it should returnLIB_BAD_PARAMSin such case - I adjusted the integration test case to this.Tests
Tests enabled:
Tests that we keep disabled:
ccm addcommand (need to investigate)Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced..github/workflows/build.ymlingtest_filter..github/workflows/cassandra.ymlingtest_filter.