Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cloud_storage_compaction_test: a couple test fixes #16284

Merged

Conversation

andrwng
Copy link
Contributor

@andrwng andrwng commented Jan 25, 2024

There were a couple issues with this test:

  • we were previously using the wrong cluster's metrics to determine whether the read replica cluster was uploading anything
  • the read replica cluster was uploading (or trying to) since the consumer offsets topic was created with the default tiered storage configuration for that cluster

Fixes #16008

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

The test was conditioned on ensuring no data was uploaded on the read
replica cluster, but it was using metrics from the source cluster to do
this check.

This fixes this to use the right cluster.
The test ensures that we don't upload from the read replica topics, but
it unintentionally does do uploads from consumer offsets topics. This
commit changes the behavior of the test to explicitly create the topic
without tiered storage.
@andrwng andrwng requested a review from dotnwat January 25, 2024 01:28
@vbotbuildovich
Copy link
Collaborator

@andrwng
Copy link
Contributor Author

andrwng commented Jan 25, 2024

/cdt

Copy link
Contributor

@andijcr andijcr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -211,12 +219,12 @@ def test_read_from_replica(self, cloud_storage_type):

# read replica success and failures
rr_upload_successes = sum([
sample.value for sample in self.redpanda.metrics_sample(
sample.value for sample in self.rr_cluster.metrics_sample(
"cloud_storage_successful_uploads",
metrics_endpoint=MetricsEndpoint.METRICS).samples
])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm curious how it was able to work, before this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check worked before, but it made its way in because cdt isn't run by default for PRs

@andrwng
Copy link
Contributor Author

andrwng commented Jan 25, 2024

The test passed in CDT

Other CI failures are unrelated

@piyushredpanda piyushredpanda merged commit bfbd456 into redpanda-data:dev Jan 26, 2024
16 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI Failure (Read Replica cluster doing uploads) in CloudStorageCompactionTest.test_read_from_replica
4 participants