HDDS-15839. Implement SCM container ID export manager by sarvekshayr · Pull Request #10813 · apache/ozone

sarvekshayr · 2026-07-20T05:30:12Z

What changes were proposed in this pull request?

Adds the SCM leader-side container ID export worker. Jobs paginate through ContainerManager.getContainerIDs() with lifecycle/health filters, write sharded text files, and pack them into a gzip-compressed TAR archive (or succeed with zero matches and no archive).

ContainerExportManager.java - Async export worker: submit, paginate, shard, TAR append, job tracking.
ExportJob.java - In-memory job state: Id, execution state, and Status (state, total rows, TAR path, error message).
ExportScope.java - Lifecycle and health filter naming for export jobs and archive file names.
ExportFileManager.java - Extended with shard write, archive creation, and failed-job cleanup helpers.
TestContainerExportManager.java - Unit tests for ContainerExportManager.
TestExportFileManager.java - Minor updates for archive path helpers.

Part 4 in splitting HDDS-15496 #10673 container ID export.

What is the link to the Apache JIRA

HDDS-15839

How was this patch tested?

Added a new unit test class TestContainerExportManager (no RPC or CLI yet).

devmadhuu

Thanks @sarvekshayr for the patch. Largely the PR looks good, just few minor comments.

szetszwo

@sarvekshayr , thanks for splitting this out. Some general comments

Refactor out all the File/Directory related code to a new class ExportFileManager
Describe directory layout in ExportFileManager (very important since changing it is an incompatible change)
Except for ContainerExportManager, use "Export" instead of "ContainerExport" as the prefix for the classnames

See also the comments inlined.

szetszwo · 2026-07-21T20:05:39Z

Refactor out all the File/Directory related code to a new class ExportFileManager
Describe directory layout in ExportFileManager (very important since changing it is an incompatible change)

@sarvekshayr , indeed, this PR is also quite big. Let's work on ExportFileManager and directory layout in a separated JIRA.

sarvekshayr · 2026-07-22T05:37:57Z

Refactor out all the File/Directory related code to a new class ExportFileManager
Describe directory layout in ExportFileManager (very important since changing it is an incompatible change)
@sarvekshayr , indeed, this PR is also quite big. Let's work on ExportFileManager and directory layout in a separated JIRA.

Created HDDS-15935.

devmadhuu

Thanks @sarvekshayr for improving the patch. Largely LGTM +1. Just a nit for rare case. Pls check.

devmadhuu

Thanks @sarvekshayr for improving the patch. LGTM +1

szetszwo · 2026-07-23T18:33:53Z

Created HDDS-15935.

@sarvekshayr , are we going to do HDDS-15935 first and then this? Is it the case?

sarvekshayr · 2026-07-24T03:12:04Z

Created HDDS-15935.
@sarvekshayr , are we going to do HDDS-15935 first and then this? Is it the case?

Not in this PR. HDDS-15935 will land in a follow-up PR after this one is merged.

ArafatKhan2198

Thanks for the patch @sarvekshayr mostly looks good!
Some comments you can consider.
Please let me know if I am missing out on something.

szetszwo

Not in this PR. HDDS-15935 will land in a follow-up PR after this one is merged.

It is fine only if this PR does not create any files.

Otherwise, we will need the disk layout description (i.e. HDDS-15935) first.

szetszwo

@sarvekshayr , Thanks for the update! Please see some quick comments inlined. BTW, the role of ContainerExportManager and ExportFileManager should be:

ContainerExportManager: manage jobs
ExportFileManager: manage files

szetszwo

@sarvekshayr , thanks for the update! Please see the comments inlined.

BTW, before submitting a PR, please review it yourself first. If there are unused methods/code, remove them.

…nd Remove unused code

szetszwo

@sarvekshayr , thanks for the update!

This change is still too big. I tried to review this multiple times but failed due to the size of the PR. (People usually cannot spend a whole day reviewing a single PR. They have other things to do. Recall the point of "Rapid Feedback" in the Google AI Overview on Multiple Small Changes.)

Please split it into two PRs.

Do not mix simple changes with complicated changes. In this case, the metrics can be added later. Tried to limit the PR size to < 30KB. The current PR size is 57KB.

% git diff apache/master...sarvekshayr/export-tool-4 > a.patch
% ls -lh a.patch
-rw-r--r--@ 1 szetszwo staff 57K Aug 13 20:48 a.patch

The test is only 15KB. It seems not enough compared to the code size (~40KB). Try using AI to generate tests.

% ls -lh hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/export/TestContainerExportManager.java
-rw-r--r--@ 1 szetszwo staff 15K Aug 13 20:51 hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/export/TestContainerExportManager.java

Do not add short methods which are used only once.

See also the comments inlined and also https://issues.apache.org/jira/secure/attachment/13083920/10813_review.patch. Sorry that I only reviewed a part of the PR due to time limit.

…re tests

sarvekshayr · 2026-08-14T07:36:47Z

Thanks @szetszwo for the review!

I agree that this PR became quite large since it pulls in all the functions for ContainerExportManager. To make reviewing easier, I propose splitting it into smaller, logical PRs as outlined below:

PR 1: Minimal end-to-end export on SCM leader - One export job can run on the SCM leader, write shards, produce a tar (or succeed with zero matches), and expose basic status.
PR 2: Export limits, request sizing, and shared constants.
PR 3: Terminal job retention and archive eviction

If this plan looks good, I’ll start refactoring and update this PR accordingly.

szetszwo · 2026-08-14T17:08:39Z

If this plan looks good, I’ll start refactoring and update this PR accordingly.

Sure, the plan looks great!

A rule of thumb is that simple change PR (renaming, adding conf/metrics, simple refactoring, etc.) can be larger while a complicated PR (logic changes, new code, non-trivial bug fixes, etc.) should be as small as possible. We want to make a complicated problem obvious and easy to be understood.

szetszwo

@sarvekshayr , thanks for the update! The current change is still quite large and also has synchronization problems (there are many synchronized methods but calling them separately will not be synchronized correctly.)

Let's do something simple first:

add ExportJob but not ContainerExportManager
change ExportFileManager
rename "shard" to "part"
rename "pageSize" to "batchSize"
rename "timestamp" to "jobStartTime" .

szetszwo · 2026-08-17T17:41:55Z

+ private final int shardSize;
+ private final int pageSize;


If I understand correctly, "shard" and "page" may not be the best names here since
shard: a specific name for horizontal partition in a database but here we don't have a database.
pageSize: it is actually a batchSize since the size information is forgotten after the file is written.
Google suggested to call the sub-files "parts".

szetszwo · 2026-08-17T17:43:03Z


+ containerExportManager = new ContainerExportManager(
+ getScmId(), containerManager, this::checkLeader, conf);
+ containerExportManager.start();


containerExportManager.start() should be called in StorageContainerManager.start() but not the constructor.

szetszwo · 2026-08-17T17:51:00Z

+ }
+
+ String shardFileName(int partIndex) {
+ return String.format("container-ids_%s_%s_part%03d.txt",


Indeed, you are already using "part" -- don't use different words for the same thing.

szetszwo · 2026-08-18T16:46:47Z

+ /**
+ * Snapshot of export progress returned to callers. Reads live fields from the enclosing job.
+ */
+ public final class Status {
+ private Status() {
+ }
+
+ public Id getId() {
+ return id;
+ }
+
+ public ExecutionState getExecutionState() {
+ return ExportJob.this.getExecutionState();
+ }
+
+ public long getTotalRows() {
+ return ExportJob.this.getTotalRows();
+ }
+
+ public String getTarPath() {
+ return ExportJob.this.getTarPath();
+ }
+
+ public String getErrorMessage() {
+ return ExportJob.this.getErrorMessage();
+ }
+ }


Remove State for now. It is currently not used and the synchronization is incorrect. Let's add it later.

szetszwo · 2026-08-18T16:47:16Z

+ private final ContainerID startContainerId;
+ private final int pageSize;
+ private final int shardSize;
+ private String tarPath;


Why tarPath is not final? Explain how could it be changed?

sarvekshayr · 2026-08-18T18:25:22Z

Raised #11052 with simple changes as suggested. Last few comments (synchronization, renaming, tarPath) will be addressed in follow-up PRs.

szetszwo · 2026-08-22T15:29:26Z

... has synchronization problems (there are many synchronized methods but calling them separately will not be synchronized correctly.)

@sarvekshayr , synchronization can be done by using a ReentrantReadWriteLock:

Whenever a thread submitting/running a job, acquire the write lock and check Job id.
When other threads getting the status, acquire the read lock.
Since we are using ReentrantReadWriteLock, remove synchronized from all the methods.

BTW, let's close this and continue with a new PR. This PR is already very long.

HDDS-15839. Implement SCM container ID export manager
2a9ece7

sarvekshayr mentioned this pull request Jul 20, 2026
HDDS-15838. Add container export shared types, config, and proto #10737
Closed

sarvekshayr requested review from ArafatKhan2198, devmadhuu and szetszwo July 20, 2026 05:37

devmadhuu reviewed Jul 20, 2026
View reviewed changes

Fixed comments in Container Export Manager
6d36753

szetszwo reviewed Jul 21, 2026
View reviewed changes

Refactored ContainerExportManager
c17dcb1

Fixed pmd error
a597a41

devmadhuu reviewed Jul 22, 2026
View reviewed changes

Comment thread...er-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ContainerExportManager.java

Set succeeded = false in catch block
a5d5e6b

sarvekshayr marked this pull request as ready for review July 22, 2026 17:06

sarvekshayr requested review from devmadhuu and szetszwo July 23, 2026 03:46

devmadhuu approved these changes Jul 23, 2026
View reviewed changes

ArafatKhan2198 reviewed Jul 24, 2026
View reviewed changes

Comment thread...er-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ContainerExportManager.java Outdated
Comment thread...er-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ContainerExportManager.java Outdated

ArafatKhan2198 reviewed Jul 24, 2026
View reviewed changes

Comment thread...er-scm/src/main/java/org/apache/hadoop/hdds/scm/container/export/ContainerExportManager.java

Better failure handling
f44789d

szetszwo requested changes Jul 24, 2026
View reviewed changes

sarvekshayr mentioned this pull request Jul 25, 2026
HDDS-15935. Extract ExportFileManager and document container export directory layout #10866
Merged

sarvekshayr marked this pull request as draft July 25, 2026 04:53

sarvekshayr added 2 commits August 2, 2026 11:24

Merge branch 'master' into export-tool-4
074c7ee

Use ExportFileManager methods
8d382a7

sarvekshayr requested a review from szetszwo August 3, 2026 06:56

szetszwo reviewed Aug 3, 2026
View reviewed changes

ContainerExportManager: manage jobs and ExportFileManager: manage files
0be4ac4

sarvekshayr requested a review from szetszwo August 5, 2026 07:23

szetszwo reviewed Aug 9, 2026
View reviewed changes

Concurrent access redesign, ExecutionState.isTerminal() on the enum a…
02e5016
…nd Remove unused code

szetszwo reviewed Aug 14, 2026
View reviewed changes

Remove ExportMetrics, refactor ExportSizing, timestamp format, add mo…
e1fd09e
…re tests

Minimal end-to-end export
730e2f4

sarvekshayr requested a review from szetszwo August 18, 2026 04:26

szetszwo reviewed Aug 18, 2026
View reviewed changes

sarvekshayr mentioned this pull request Aug 18, 2026
HDDS-16219. Add ExportJob and expand ExportFileManager #11052
Merged

szetszwo closed this Aug 22, 2026

sarvekshayr mentioned this pull request Aug 24, 2026
HDDS-15839. Implement SCM container ID export manager #11097
Open

Uh oh!

Conversation

sarvekshayr commented Jul 20, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

devmadhuu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo commented Jul 21, 2026

Uh oh!

sarvekshayr commented Jul 22, 2026

Uh oh!

devmadhuu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devmadhuu left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Jul 23, 2026

Uh oh!

sarvekshayr commented Jul 24, 2026

Uh oh!

ArafatKhan2198 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szetszwo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarvekshayr commented Aug 14, 2026

Uh oh!

szetszwo commented Aug 14, 2026

sarvekshayr commented Jul 20, 2026•
edited
Loading

szetszwo left a comment •
edited
Loading

szetszwo left a comment •
edited
Loading

szetszwo commented Aug 22, 2026•
edited
Loading