Implement `cass_retry_policy_logging_new` and enable some exec profile integration tests #287

muzarski · 2025-04-28T11:53:10Z

The logging retry policy is used in tests in order to observe and verify retry decisions.
Therefore, implementing it allows us enable more integration tests.

As the semantics of retry policy differ between C++ driver and Rust driver, the logs are a bit different, so the tests need to be adjusted.

C++:

// On request error
    LOG_INFO("Retrying on request error at consistency %s (initial consistency: %s, error: %s, "
               "retries: %d)",

    LOG_INFO("Ignoring request error (initial consistency: %s, error: %s, retries: %d)",
               cass_consistency_string(cl), error->message().to_string().c_str(), num_retries);

// On unavailable
    LOG_INFO("Retrying on unavailable error at consistency %s (initial consistency: %s, required "
               "replica: %d, alive replica: %d, retries: %d)",
               cass_consistency_string(decision.retry_consistency()), cass_consistency_string(cl),
               required, alive, num_retries);

    LOG_INFO("Ignoring unavailable error (initial consistency: %s, required replica: %d, alive "
               "replica: %d, retries: %d)",
               cass_consistency_string(cl), required, alive, num_retries);

// On write timeout
    LOG_INFO("Retrying on write timeout at consistency %s (initial consistency: %s, required "
               "acknowledgments: %d, received acknowledgments: %d, write type: %s, retries: %d)",
               cass_consistency_string(decision.retry_consistency()), cass_consistency_string(cl),
               required, received, cass_write_type_string(write_type), num_retries);

    LOG_INFO("Ignoring write timeout (initial consistency: %s, required acknowledgments: %d, "
               "received acknowledgments: %d, write type: %s, retries: %d)",
               cass_consistency_string(cl), required, received, cass_write_type_string(write_type),
               num_retries);

// On read timeout
     LOG_INFO("Retrying on read timeout at consistency %s (initial consistency: %s, required "
               "responses: %d, received responses: %d, data retrieved: %s, retries: %d)",
               cass_consistency_string(decision.retry_consistency()), cass_consistency_string(cl),
               required, received, data_recevied ? "true" : "false", num_retries);
     
     LOG_INFO("Ignoring read timeout (initial consistency: %s, required responses: %d, received "
               "responses: %d, data retrieved: %s, retries: %d)",
               cass_consistency_string(cl), required, received, data_recevied ? "true" : "false",
               num_retries);

CPP-Rust (based on Rust driver's semantics):

match &decision {
      RetryDecision::RetrySameTarget(consistency) => tracing::info!(
          "Retrying on the same target; Error: {}; Initial Consistency: {:?}; New Consistency: {:?}",
          error,
          initial_consistency,
          consistency
      ),
      RetryDecision::RetryNextTarget(consistency) => tracing::info!(
          "Retrying on the next target; Error: {}; Initial Consistency: {:?}; New Consistency: {:?}",
          error,
          initial_consistency,
          consistency
      ),
      RetryDecision::IgnoreWriteError => {
          tracing::info!("Ignoring write error; Error: {}", error)
      }

Fixes: #260
Ref: #132

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
I have implemented Rust unit tests for the features/changes introduced.
I have enabled appropriate tests in Makefile in gtest_filter.

In the next commits, I'll want to import RetryPolicy from rust-driver. 1. We don't want to have a name clashes 2. We already have a convention of prefixing public types with Cass. Bonus: I also removed the global variant imports.

wprzytula · 2025-06-04T09:33:13Z

Rebased on master.

tests/src/integration/tests/test_exec_profile.cpp

The prefix is redundant and makes it harder to read the code.

It's used in some tests, that's why we need to implement it. The semantics of this retry policies is a bit different in C++ driver and in Rust driver. Therefore, the logs are not exactly the same. So in order to enable the tests using this retry policy, we will need to adjust the tests to expect the new format of the logs.

It was commented out, because `cass_retry_policy_logging_new` was not implemented.

IgnoringRetryPolicy is a retry policy that always ignores all errors. It's implemented as another variant of CassRetryPolicy, and it is going to be used in integration tests. There is one test in particular, `ExecutionProfileTest.Integration_Cassandra_RetryPolicy`, which requires this retry policy to be able to run. In order to enable that test and not change the retry policy it uses, we need to implement it. The next commits will implement the bridge between Rust and C++ integration testing module, so that the test can use this retry policy.

IgnoreRetryPolicy is now exposed to C++ integration tests. It can be used in tests just as the tests expect it to be used.

In 78245ba 3 years ago, the test was made use DefaultRetryPolicy, instead of IgnoreRetryPolicy, in order the code to compile. Now that IgnoreRetryPolicy is supported, we make the test use it again. This allows us to enable the test.

wprzytula · 2025-06-06T13:53:04Z

Addressed comments. The test now uses IgnoreRetryPolicy.

Lorak-mmk

Thanks for implementing the ignore policy!

Lorak-mmk · 2025-06-06T14:08:02Z

scylla-rust-wrapper/src/batch.rs

        ArcFFI::as_ref(retry_policy).map(|policy| match policy {
-            CassRetryPolicy::DefaultRetryPolicy(default) => {
+            CassRetryPolicy::Default(default) => {
                default.clone() as Arc<dyn scylla::policies::retry::RetryPolicy>
            }
-            CassRetryPolicy::FallthroughRetryPolicy(fallthrough) => fallthrough.clone(),
-            CassRetryPolicy::DowngradingConsistencyRetryPolicy(downgrading) => downgrading.clone(),
+            CassRetryPolicy::Fallthrough(fallthrough) => fallthrough.clone(),
+            CassRetryPolicy::DowngradingConsistency(downgrading) => downgrading.clone(),
        });


Commit: "CassRetryPolicy: cut RetryPolicy- off enum variants "

We have the same problem as in Rust Driver: clippy by default does not trigger some lints for public items. Please apply the same solution as in the Rust Driver (it was some config option).

I did notice it and I'm going to fix it in a follow-up PR.

muzarski marked this pull request as draft April 28, 2025 11:53

muzarski self-assigned this Apr 28, 2025

muzarski added the P2 P2 item - probably some people use this, let's implement that label Apr 28, 2025

muzarski mentioned this pull request May 28, 2025

cpp-rust-driver test parity with original cpp-driver #132

Open

muzarski force-pushed the retry-policy-logging branch 3 times, most recently from 84abc8b to 07263b3 Compare May 30, 2025 10:08

retry_policy: rename RetryPolicy to CassRetryPolicy

2825fd6

In the next commits, I'll want to import RetryPolicy from rust-driver. 1. We don't want to have a name clashes 2. We already have a convention of prefixing public types with Cass. Bonus: I also removed the global variant imports.

wprzytula force-pushed the retry-policy-logging branch from 07263b3 to 4fed103 Compare June 4, 2025 09:32

wprzytula marked this pull request as ready for review June 4, 2025 09:33

wprzytula requested review from wprzytula and Lorak-mmk June 4, 2025 09:33

wprzytula approved these changes Jun 4, 2025

View reviewed changes

wprzytula added this to the 0.5 milestone Jun 4, 2025

Lorak-mmk reviewed Jun 6, 2025

View reviewed changes

tests/src/integration/tests/test_exec_profile.cpp Outdated Show resolved Hide resolved

wprzytula and others added 7 commits June 6, 2025 15:24

CassRetryPolicy: cut RetryPolicy- off enum variants

18211da

The prefix is redundant and makes it harder to read the code.

testing: remove cass_retry_policy_logging_new from unimplemented

55474ae

IT: uncomment the code related to logging retry policy

894903d

It was commented out, because `cass_retry_policy_logging_new` was not implemented.

testing: Bridge IgnoreRetryPolicy to C++ integration

cc6e28f

IgnoreRetryPolicy is now exposed to C++ integration tests. It can be used in tests just as the tests expect it to be used.

wprzytula force-pushed the retry-policy-logging branch from 4fed103 to f5bb355 Compare June 6, 2025 13:52

wprzytula requested a review from Lorak-mmk June 6, 2025 13:52

Lorak-mmk approved these changes Jun 6, 2025

View reviewed changes

wprzytula merged commit f5de750 into scylladb:master Jun 6, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement `cass_retry_policy_logging_new` and enable some exec profile integration tests #287

Implement `cass_retry_policy_logging_new` and enable some exec profile integration tests #287

Uh oh!

muzarski commented Apr 28, 2025 •

edited by wprzytula

Loading

Uh oh!

wprzytula commented Jun 4, 2025

Uh oh!

Uh oh!

wprzytula commented Jun 6, 2025

Uh oh!

Lorak-mmk left a comment

Uh oh!

Lorak-mmk Jun 6, 2025

Uh oh!

wprzytula Jun 6, 2025

Uh oh!

Uh oh!

Uh oh!

Implement cass_retry_policy_logging_new and enable some exec profile integration tests #287

Implement cass_retry_policy_logging_new and enable some exec profile integration tests #287

Uh oh!

Conversation

muzarski commented Apr 28, 2025 • edited by wprzytula Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-review checklist

Uh oh!

wprzytula commented Jun 4, 2025

Uh oh!

Uh oh!

wprzytula commented Jun 6, 2025

Uh oh!

Lorak-mmk left a comment

Choose a reason for hiding this comment

Uh oh!

Lorak-mmk Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

wprzytula Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Implement `cass_retry_policy_logging_new` and enable some exec profile integration tests #287

Implement `cass_retry_policy_logging_new` and enable some exec profile integration tests #287

muzarski commented Apr 28, 2025 •

edited by wprzytula

Loading