Repository navigation
HDDS-15839. Implement SCM container ID export manager - #11097
Conversation
|
Supersedes #10813. |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@sarvekshayr , thanks for the update! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13084134/11097_review.patch
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@sarvekshayr , thanks for the update! Two quick comments inlined.
…ds and methods for batching
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@sarvekshayr , thanks for the update! Please see the comments inlined.
|
@szetszwo could you please review the PR when you have a moment? |
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
Thanks for working on this, @sarvekshayr!
I’ve left a few comments. Please feel free to close them if they’re already covered or if I’ve missed something.
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
Thanks for working on this @sarvekshayr
Most of the changes look good.
few final comments -
| for (File part : parts) { | ||
| if (!isLeaderReady.getAsBoolean()) { | ||
| // close() would publish the partial archive; abort() discards the temp file instead. | ||
| atomicOut.abort(); | ||
| throw new IOException("SCM lost leadership while archiving " + job); | ||
| } | ||
| Archiver.includeFile(part, part.getName(), tarOut); | ||
| } | ||
| } |
There was a problem hiding this comment.
The code checks leadership before copying each file. If leadership changes while the final file is being copied, there is no further check. Closing the streams publishes the archive, and the job reports success.
Fix: Add a final leadership/cancellation check after the loop, before closing publishes the archive. Abort the temporary output if the check fails.
Test improvement: The new test at line 353 supplies () -> false, so it tests rejection before copying begins. Add a one-file test where the first check returns true and subsequent checks return false; expect failure and no archive.
There was a problem hiding this comment.
Archiving is the last step of a long-running job. Failing it because leadership flipped in the final few milliseconds throws away a correct archive. Keeping it costs the operator at most one extra archive from the old leader. The contents may be slightly stale, which the class javadoc already says (best-effort, not point-in-time).
ArafatKhan2198
left a comment
ArafatKhan2198
left a comment
There was a problem hiding this comment.
Thanks for working on this @sarvekshayr
LGTM +1
|
@szetszwo any comments from your end? |
|
Thanks @szetszwo and @ArafatKhan2198 for the review. |
3 participants
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).ExportFileManager.java- Extended with shard write, archive creation, and failed-job cleanup helpers.What is the link to the Apache JIRA
HDDS-15839
How was this patch tested?
Added a new unit test class
TestContainerExportManager.