AlexanderKM opened a new pull request, #19337:
URL: https://github.com/apache/pinot/pull/19337
# Remove unsafe delete-before-copy in S3PinotFS#copyDir
## What
`S3PinotFS#copyDir`'s single-file branch used to do:
```java
if (!isDirectory(srcUri)) {
delete(dstUri, true);
return copyFile(srcUri, dstUri);
}
```
This deletes the destination object before copying the new one in. If the
process (controller, server,
minion, etc.) crashes or OOMs after the delete but before the copy
completes, the destination object is
gone permanently — nothing ever replaces it.
The fix removes the delete and relies on S3's `CopyObject` (and `PutObject`)
semantics: writing to an
existing key is a single atomic operation that replaces the object in place.
There's no need to clear the
destination first, and doing so only introduces a window where data can be
lost.
```java
if (!isDirectory(srcUri)) {
return copyFile(srcUri, dstUri);
}
```
## Why this is safe
- S3 object writes (`PutObject`/`CopyObject`) to an existing key are atomic
— the destination key is
either the old content or the new content, never neither. S3 has had
strong read-after-write consistency
for both new and overwritten keys since Dec 2020, so there is no
partial-write or stale-read concern.
- This pattern is already how `GcsPinotFS#copy` handles the same case (see
its single-file branch) — it
never deletes the destination first, because GCS object copy is also an
atomic overwrite. This change
brings S3 in line with that existing, working implementation.
- The removed `delete()` call was unconditional and didn't check whether the
destination was actually a
directory — it wasn't a deliberate "convert directory to file" safeguard,
just a leftover pre-clear that
has been present since the very first version of the S3 plugin (predates
`copyDir` itself, back when this
method was just `copy()`).
## Blast radius / what this affects
`copyDir`'s single-file path is also what `doMove` uses under the hood
(`doMove` = `copyDir` + delete
source), so every S3 `copy`/`move` call in the codebase was exposed to this
race. Tracing call sites:
- **Controller METADATA push** (`ZKOperator#copyFromSegmentURIToDeepStore`)
— when a segment is pushed
with `uploadType == METADATA`, the client sends only metadata plus a
`sourceDownloadURI`, and the
**controller itself** copies the segment bytes from that staging location
to the table's deep-store
location. This is the path minion tasks (purge,
`RealtimeToOfflineSegmentsTask`, `MergeRollupTask`,
etc.) rely on when they stage a derived/cleaned segment and use METADATA
push to hand it to the
controller — but it is a general METADATA-push code path, not
minion-specific, so any client using
METADATA push against an S3 deep store was exposed.
- **Controller realtime (LLC) segment commit/repair**
(`PinotLLCRealtimeSegmentManager#moveSegmentFile`) —
moves a just-committed consuming segment file, and LLC segment
repair/re-upload, from a temp location to
its permanent controller-managed deep-store path.
- **Controller segment deletion/retention**
(`SegmentDeletionManager#moveSegmentsToDeletedDir`) — moves
segments to a `DELETED_SEGMENTS` staging directory instead of
hard-deleting, so `RetentionManager` can
purge them later.
- **Batch ingestion segment generation jobs** (`SegmentGenerationJobUtils`,
used by the Hadoop, Spark, and
standalone job runners) — moves locally-built segment tar files from a
local/staging directory to the
final output deep-store directory before push.
Before this fix, a crash in any of these flows at the wrong moment could
leave an S3 deep store missing a
segment (or a deleted-segment/purge artifact) with no way to recover it
short of re-running the upstream
job. This fix closes that window across all of them, since they all bottom
out in `S3PinotFS#copyDir`.
## Testing
- Existing unit tests in `S3PinotFSCopyTest` continue to cover single-file
copy request construction.
- No behavior change for the happy path — `CopyObjectRequest` is issued
exactly as before; only the
preceding `DeleteObjectRequest` is removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
