scylladb / cpp-rust-driver

API-compatible rewrite of https://github.com/scylladb/cpp-driver as a wrapper for Rust driver.
GNU Lesser General Public License v2.1
11 stars 11 forks source link

Execution profiles support #107

Closed wprzytula closed 1 year ago

wprzytula commented 1 year ago

The Rust driver has had recently added support for Execution Profiles. As that concept is present in the CPP driver, this PR implements the bridge between them (as there are, of course, some differences to be handled). As a side effect, this PR raises the supported Rust driver version to a recent one, featuring a small UDT refactor, a major load balancing refactor and support for Serverless Cloud connections.

This PR contains a considerable number of tests, including unit tests and integration tests (to which end proxy is used). The existing integration tests inherited from CPP driver are (for various reasons) not suitable for use in bindings (see listing below).

Existing integration tests:

Fixes: #38

wprzytula commented 1 year ago

Some of the commits do not compile successfully (e.g. updating the Rust driver version). * The PR description mentions that new tests are added to the build and cassandra workflows but the files are unmodified.

I see no way to have both small, logically separate commits and commits that all compile. The bump of Rust driver version is a monolith, which changes API a lot and hence we can't expect the next commits to compile - they have to fix many issues before.

Please add a "fixes" annotation to the description that the PR will fix CassExecProfile #38.

Done.

Have you tried to run tests in test_exec_profile, can we fix and enable them in CI? I think we can also add Rust tests in CI, however, cargo test generated a lot of warnings that should be suppressed.

About cargo test in CI: done. About existing tests: I've described them in detail in the PR's description.

In the examples catalog we have the execution_profiles example, have you tried to run it with valgrind to check memory corruption errors?

Yes, these are the results, so I assume everything's all right:

LEAK SUMMARY:
    definitely lost: 0 bytes in 0 blocks
    indirectly lost: 0 bytes in 0 blocks
    possibly lost: 123,671 bytes in 1,162 blocks
    still reachable: 548,741 bytes in 676 blocks
    suppressed: 0 bytes in 0 blocks
piodul commented 1 year ago

Some of the commits do not compile successfully (e.g. updating the Rust driver version). * The PR description mentions that new tests are added to the build and cassandra workflows but the files are unmodified.

I see no way to have both small, logically separate commits and commits that all compile. The bump of Rust driver version is a monolith, which changes API a lot and hence we can't expect the next commits to compile - they have to fix many issues before.

if rust driver version bump is such a big effort then it should be done in a separate PR. If the adjustments required for the new version are straightforward and/or independent from each other (I's expect them to be), then it's fine to do it in a single commit.

wprzytula commented 1 year ago

v2:

wprzytula commented 1 year ago

v2.1 - fixed memory leak due to an oversight during updating Rust driver's version