datalake: clean up files after translation error by andrwng · Pull Request #27737 · redpanda-data/redpanda

andrwng · 2025-09-25T20:53:14Z

We previously didn't clean up files record_multiplexer::finish()
returned errors. Since the returned success result is previously what
held the local files, this commit updates the signature to collect the
files in finish() (regardless of whether the operation succeeds or not),
and cleans up the files in translation_task::finish() and ::discard().

Backports Required

Release Notes

Bug Fixes

Redpanda will now properly clean up discarded on-disk files on certain error cases in the Iceberg topics write path.

This will be useful to inject transient errors to the record multiplexer (e.g. schema registry errors) to fail translation, rather than DLQ records. Will be used in an upcoming test to ensure we clean up local files after such a failure.

bharathv

lgtm few nits, nice find.

bharathv · 2025-09-25T22:23:01Z

src/v/datalake/record_multiplexer.h

     * progress.
     */
-    ss::future<result<write_result, writer_error>> finish() &&;
+    ss::future<result<write_result, writer_error>> finish(finished_files*) &&;


any reason to pass a pointer vs passing a reference instead? (can never be null?).

Nah not really, habit from an older code style where outparams were pointers. Will change

bharathv · 2025-09-25T22:25:14Z

src/v/datalake/translation_task.cc

+          mux_result.error(),
+          files.data_files.size(),
+          files.dlq_files.size());
+        [[maybe_unused]] auto del_res = co_await delete_data_and_dlq_files(


nit: auto _ = ?

bharathv · 2025-09-25T22:27:05Z

src/v/datalake/translation_task.cc

  ss::abort_source& as) && {
-    auto mux_result = co_await std::move(_multiplexer).finish();
+    record_multiplexer::finished_files files;
+    auto mux_result = co_await std::move(_multiplexer).finish(&files);


oof nice find.

We previously didn't clean up files record_multiplexer::finish() returned errors. Since the returned success result is previously what held the local files, this commit updates the signature to collect the files in finish() (regardless of whether the operation succeeds or not), and cleans up the files in translation_task::finish() and ::discard().

Copilot

Pull Request Overview

This PR fixes a bug where on-disk files were not properly cleaned up when the datalake record multiplexer's finish() method returned errors. The fix modifies the signature to ensure files are collected and cleaned up regardless of operation success.

Key changes:

Modified record_multiplexer::finish() to accept a finished_files output parameter for collecting files regardless of success/failure
Added cleanup logic in translation_task::finish() and ::discard() to delete local files when errors occur
Updated all test files to accommodate the new finish() signature

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/v/datalake/translation_task.cc	Core fix: adds file cleanup on error and updates finish/discard methods to use new multiplexer signature
src/v/datalake/record_multiplexer.h	Changes multiplexer interface to accept finished_files output parameter
src/v/datalake/record_multiplexer.cc	Updates implementation to populate finished_files parameter instead of result struct
src/v/datalake/record_schema_resolver.h	Adds test resolver class for injecting errors in tests
src/v/datalake/record_schema_resolver.cc	Implements test resolver error injection functionality
src/v/datalake/tests/translation_task_test.cc	Adds comprehensive tests for cleanup behavior and updates existing test setup
src/v/datalake/tests/record_multiplexer_test.cc	Updates all test calls to use new finish() signature
src/v/datalake/tests/record_multiplexer_bench.cc	Updates benchmark code for new signature
src/v/datalake/tests/gtest_record_multiplexer_test.cc	Updates Google Test cases for new signature

Copilot · 2025-09-25T22:38:20Z

src/v/datalake/translation_task.cc

+    }
+    if (dlq_result.has_error()) {
+        vlog(
+          log.warn, "error deleting local dlq files - {}", data_result.error());


Wrong variable used in error message. Should be dlq_result.error() instead of data_result.error() since this is logging the DLQ file deletion error.

Suggested change

log.warn, "error deleting local dlq files - {}", data_result.error());

log.warn, "error deleting local dlq files - {}", dlq_result.error());

vbotbuildovich · 2025-09-26T01:47:05Z

CI test results

test results on build#73007

test_class	test_method	test_arguments	test_kind	job_url	test_status	passed	reason	test_history
ClusterRateQuotaTest	test_client_response_throttle_mechanism	null	integration	https://buildkite.com/redpanda/redpanda/builds/73007#0199832d-d664-46db-a9e6-f89c66228d7f	FLAKY	13/21	upstream reliability is '92.0103092783505'. current run reliability is '61.904761904761905'. drift is 30.10555 and the allowed drift is set to 50. The test should PASS	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=ClusterRateQuotaTest&test_method=test_client_response_throttle_mechanism
DatalakeBlockedCatalogTest	test_disable_reenable_after_blocking	{"cloud_storage_type": 1}	integration	https://buildkite.com/redpanda/redpanda/builds/73007#0199832e-0807-4ff7-af92-199c7565001c	FLAKY	20/21	upstream reliability is '100.0'. current run reliability is '95.23809523809523'. drift is 4.7619 and the allowed drift is set to 50. The test should PASS	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=DatalakeBlockedCatalogTest&test_method=test_disable_reenable_after_blocking
RedpandaNodeOperationsSmokeTest	test_node_ops_smoke_test	{"cloud_storage_type": 1, "mixed_versions": false}	integration	https://buildkite.com/redpanda/redpanda/builds/73007#0199832e-0807-4ff7-af92-199c7565001c	FLAKY	20/21	upstream reliability is '100.0'. current run reliability is '95.23809523809523'. drift is 4.7619 and the allowed drift is set to 50. The test should PASS	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=RedpandaNodeOperationsSmokeTest&test_method=test_node_ops_smoke_test
TimeQueryTest	test_timequery_with_local_gc	null	integration	https://buildkite.com/redpanda/redpanda/builds/73007#0199832d-d65d-43be-9770-6d8ef85dcb7b	FLAKY	19/21	upstream reliability is '97.11340206185567'. current run reliability is '90.47619047619048'. drift is 6.63721 and the allowed drift is set to 50. The test should PASS	https://redpanda.metabaseapp.com/dashboard/87-tests?tab=142-dt-individual-test-history&test_class=TimeQueryTest&test_method=test_timequery_with_local_gc

vbotbuildovich · 2025-09-26T01:47:37Z

/backport v25.2.x

vbotbuildovich · 2025-09-26T01:47:39Z

/backport v25.1.x

vbotbuildovich · 2025-09-26T01:48:44Z

Failed to create a backport PR to v25.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-27737-v25.2.x-563 remotes/upstream/v25.2.x
git cherry-pick -x c253a2e139 fd496bc96c

Workflow run logs.

vbotbuildovich · 2025-09-26T01:48:50Z

Failed to create a backport PR to v25.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-27737-v25.1.x-868 remotes/upstream/v25.1.x
git cherry-pick -x c253a2e139 fd496bc96c

Workflow run logs.

dotnwat

nice. makes sense.

datalake: add test-only type resolver

c253a2e

This will be useful to inject transient errors to the record multiplexer (e.g. schema registry errors) to fail translation, rather than DLQ records. Will be used in an upcoming test to ensure we clean up local files after such a failure.

andrwng requested review from bharathv, Copilot and nvartolomei and removed request for bharathv and Copilot September 25, 2025 20:53

github-actions bot added the area/redpanda label Sep 25, 2025

andrwng requested review from bharathv and dotnwat September 25, 2025 20:53

bharathv previously approved these changes Sep 25, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings September 25, 2025 22:37

andrwng dismissed bharathv’s stale review via fd496bc September 25, 2025 22:37

andrwng force-pushed the datalake-error-cleanup branch from f24f8fb to fd496bc Compare September 25, 2025 22:37

Copilot AI reviewed Sep 25, 2025

View reviewed changes

andrwng requested a review from bharathv September 25, 2025 22:39

andrwng enabled auto-merge September 25, 2025 22:41

bharathv approved these changes Sep 25, 2025

View reviewed changes

andrwng merged commit 14fd1c5 into redpanda-data:dev Sep 26, 2025
17 checks passed

vbotbuildovich mentioned this pull request Sep 26, 2025

[v25.2.x] datalake: clean up files after translation error #27745

Open

vbotbuildovich mentioned this pull request Sep 26, 2025

[v25.1.x] datalake: clean up files after translation error #27746

Open

dotnwat reviewed Sep 26, 2025

View reviewed changes

This was referenced Sep 26, 2025

[v25.2.x] datalake: clean up files after translation error #27765

Merged

[v25.1.x] datalake: clean up files after translation error #27766

Merged

	log.warn, "error deleting local dlq files - {}", data_result.error());
	log.warn, "error deleting local dlq files - {}", dlq_result.error());

Conversation

andrwng commented Sep 25, 2025

Backports Required

Release Notes

Bug Fixes

Uh oh!

bharathv left a comment

Choose a reason for hiding this comment

Uh oh!

bharathv Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

andrwng Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

bharathv Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

andrwng Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

bharathv Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

vbotbuildovich commented Sep 26, 2025

CI test results

Uh oh!

Uh oh!

vbotbuildovich commented Sep 26, 2025

Uh oh!

vbotbuildovich commented Sep 26, 2025

Uh oh!

vbotbuildovich commented Sep 26, 2025

Uh oh!

vbotbuildovich commented Sep 26, 2025

Uh oh!

dotnwat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants