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: Fix transactions e2e test #6001

Merged
merged 7 commits into from
Aug 15, 2022

Conversation

Lazin
Copy link
Contributor

@Lazin Lazin commented Aug 12, 2022

Cover letter

Fix test_restore_with_aborted_tx test failure on CI.

Use transactions.timeout.ms in kafka producer and add delay before reconnect.
Fix segment hydration logic remote_segment. Currently, the tx-manifest is never saved to SI cache. This PR fixes this so the materialization of transactional metadata could be done from cache instead of S3.

Backport Required

  • not a bug fix
  • papercut/not impactful enough to backport
  • v22.2.x
  • v22.1.x
  • v21.11.x

UX changes

none

Release notes

Improvements

  • Improve transaction metadata handling in Shadow Indexing

Log frequent message on trace instead of debug.
Extend prefix logger header with segment base/max offsets.
Currently, tx-manifests are downloaded every time. Because of that we
will make a request to S3 when the remote_segment is materialized.
Even if transactions are not used we will still have to make a request
to get NoSuchKey response. To avoid this this commit adds logic that
seralizes the manifest to SI cache.
@mmedenjak mmedenjak added kind/bug Something isn't working ci-failure area/cloud-storage Shadow indexing subsystem and removed area/redpanda labels Aug 13, 2022
ztlpn
ztlpn previously approved these changes Aug 13, 2022
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

lgtm but last commit could use an expanded commit message.

@@ -216,6 +219,7 @@ def done():
producer.produce(self.topic)
except ck.KafkaException as err:
self.logger.warn(f"producer error: {err}")
time.sleep(10)
Copy link
Member

Choose a reason for hiding this comment

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

Use short timeout if kafka producer returned an error. Add exception for
BadLogLines.

This commit message is describing the literal introduction of the sleep, not why the sleep is being added. Can you add the why to the commit message?

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

Use short timeout if kafka producer returned an error.
This is needed to guarantee that the previous transaction was aborted.
The added delay is longer than the transaction's timeout.

Add exception for BadLogLines.
@Lazin Lazin merged commit 41dd9d8 into redpanda-data:dev Aug 15, 2022
@Lazin
Copy link
Contributor Author

Lazin commented Aug 15, 2022

\backport v22.1.x

@Lazin Lazin deleted the fix/transactions-e2e-test branch August 15, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cloud-storage Shadow indexing subsystem area/redpanda ci-failure kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants