Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
7dc467a to
47a39f8
Compare
|
run buildall |
|
/review |
TPC-H: Total hot run time: 17015 ms |
TPC-DS: Total hot run time: 82202 ms |
ClickBench: Total hot run time: 14.57 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Capped static review of exact head 47a39f8de697291c14092d7c2b2f333e8940196c against base 96d0ac68e84f92ed71acaad13560c076231a9266. Six verified findings remain (2 P1, 4 P2). The maximum third review round added the final settled-state finding, so this is truthfully capped/incomplete rather than a converged no-new-findings result.
Goal and scope: the stable ASCII happy path does batch matched S3 files lexically and reaches FINISHED after a successful final commit, and the change remains focused. The goal is not ready overall because fresh-empty lifecycle, terminal-state concurrency, source scoping, mixed-version leadership, cloud final-offset persistence, and recovered-task cleanup are incorrect.
Concurrency and lifecycle: transaction callback ordering, recurring metadata work, delayed task scheduling, manual pause/stop/resume, cancellation, and all three new completion sites were traced. FINISHED is not absorbing and eligibility-to-FINISHED transitions are not atomic; recovered completion also leaves a PENDING placeholder task and stale retry message.
Persistence and compatibility: current-version legacy images default to LEXICAL, provider reconstruction order is correct, the default offset-provider interface preserves other providers, and stable local/cloud committed offsets were traced. However, an old electable FE silently reinterprets a new ONCE job as lexical polling, and cloud recovery can checkpoint a FINISHED job with stale offsetProviderPersist. No FE-BE wire format, process config, or database storage format changes were introduced; the behavior-defining job property is persisted in the generic job map.
Configuration and parallel paths: ONCE is immutable through ALTER and rejects explicit offsets for S3, but validation occurs before source resolution, so CDC and FROM-source JDBC accept the S3-only setting as a silent no-op and can have valid offset configuration rejected.
Tests and other checkpoints: the changed unit/regression tests cover stable batching and uncontended completion, but omit end-to-end empty-source state, latch-based terminal races, non-S3 negative commands, mixed-version replay/failover, cloud recovery plus image round-trip, and recovered placeholder cleanup. No local build or test execution was permitted; the author reports 19 unit tests, while regression/manual testing are unchecked in the PR body. No material new performance issue was found in the bounded pagination path. Logging exists, but final offset/task observability is incorrect as noted. The new user-visible property still needs documentation, which the PR already acknowledges. No additional user review focus was provided.
|
|
||
| Env.getCurrentEnv().getJobManager().getStreamingTaskManager().removeRunningTask(task); | ||
| if (offsetProvider.hasReachedEnd()) { | ||
| if (offsetProvider.hasReachedEnd(task.getRunningOffset())) { |
There was a problem hiding this comment.
[P1] Make ONCE completion terminal across scheduler races
The commit thread holds the job lock through this block, but the recurring metadata task performs S3 I/O before acquiring it. If that listing fails while the final batch commits, this block sets FINISHED and removes the transaction callback; the waiting fetchMeta path then changes the job to PAUSED because AbstractJob permits FINISHED -> PAUSED. Auto/manual resume can reopen a job whose callback is no longer registered. Both new scheduler completion paths have the inverse TOCTOU: their earlier eligibility checks are released before an unconditional FINISHED write, so they can overwrite a concurrent PAUSED/STOPPED state. Please make these transitions conditional under the same job lock, or enforce that terminal states are absorbing, and add latch-based coverage.
| public static SourceOffsetProvider createSourceOffsetProvider( | ||
| String sourceType, StreamingJobProperties jobProperties) { | ||
| try { | ||
| if ("s3".equalsIgnoreCase(sourceType) && jobProperties.isS3OnceMode()) { |
There was a problem hiding this comment.
[P1] Fence ONCE jobs from older FE leaders
The mode is persisted only as a key in the generic properties map. An FE at the base version replays that map without validating unknown keys, then its one-argument factory always reconstructs the no-arg lexical S3 provider. During a rolling FE upgrade, an older follower that becomes leader can therefore turn a PENDING ONCE job into indefinite polling and ingest later files instead of reaching FINISHED. Please gate creation until every electable FE supports this mode, or persist a backward-compatible discriminator that makes old leaders fail closed, with a mixed-version replay/failover test.
| GlobListing globListing = fileSystem.globListWithLimit(Location.of(filePath), startFile, 1, 1); | ||
| if (!globListing.getFiles().isEmpty() && StringUtils.isNotEmpty(globListing.getMaxFile())) { | ||
| boolean hasFiles = !globListing.getFiles().isEmpty(); | ||
| if (onceMode && startFile != null && !hasFiles) { |
There was a problem hiding this comment.
[P2] Keep an initially empty ONCE source out of the failure budget
With no committed offset, this branch deliberately leaves reachedEnd false, while hasMoreDataToConsume returns true for currentOffset == null. The PENDING path does not probe metadata, so a normal empty glob is dispatched; getNextOffset throws, the task retries four times, and repeated auto-resumes can end in CANNOT_RESUME_ERR. ONCE should either finish an empty snapshot or wait through the no-data delay path, but it should not treat zero matches as a task failure. Please cover the full PENDING -> scheduler -> task state transition, not only the raw provider exception.
| && v <= (long) (1024 * 1024 * 1024) * 10, | ||
| StreamingJobProperties.S3_MAX_BATCH_BYTES_PROPERTY + " should between 100MB and 10GB"); | ||
|
|
||
| String ingestionMode = getS3IngestionMode(); |
There was a problem hiding this comment.
[P2] Reject the S3-only mode on non-S3 sources
This validation runs before the source type is resolved, so cdc_stream and FROM-source JDBC jobs accept s3.ingestion_mode=ONCE even though their provider paths ignore it and continue indefinitely. The same irrelevant property can still reject those providers' supported offset setting here. Please validate the explicit property after source/provider resolution in CREATE and ALTER, rejecting it outside S3, and add negative coverage for both non-S3 paths.
| } | ||
| // reject task if no more data to consume | ||
| if (!job.hasMoreDataToConsume()) { | ||
| if (job.hasReachedEnd()) { |
There was a problem hiding this comment.
[P2] Persist the cloud-replayed offset before finishing
After a crash following a cloud commit, replayOnCloudMode restores the authoritative offset only into offsetProvider; unlike local replay, it does not refresh offsetProviderPersist. This new exhaustion branch can then journal FINISHED with the stale/null persisted string. Because Gson excludes the provider object and a restored FINISHED job does not replay Meta Service again, a later checkpoint restart permanently shows the old final offset. Please synchronize offsetProviderPersist whenever cloud replay applies an attachment before allowing either natural-completion path to persist FINISHED, and add a cloud recovery plus image-round-trip test.
| if (job.hasReachedEnd()) { | ||
| job.updateJobStatus(JobStatus.FINISHED); | ||
| job.logUpdateOperation(); | ||
| return; |
There was a problem hiding this comment.
[P2] Retire the placeholder task when recovery finishes
A recovered ONCE job records a PENDING placeholder task; its first scheduler pass delays that task and sets the "will retry" runtime message. After the metadata probe marks the source exhausted, the delayed pass reaches this return without executing, canceling, finalizing, or removing the task and without clearing the message. The job is therefore shown as FINISHED while its task remains PENDING with no finish time and the job still says it will retry. Please finalize/cancel the placeholder and clear its reference/message as part of the terminal transition, with a recovered-empty-tail state test.
FE UT Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: S3 streaming insert jobs currently keep polling for new files after consuming all files matched by the path. This change adds
s3.ingestion_mode=ONCE, which imports matching files in lexical batches and marks the job asFINISHEDafter the final batch commits successfully. Recovered jobs probe from their committed offset and finish when no files remain. ONCE jobs reject user-specified offsets, including offset changes through ALTER JOB.Release note
S3 streaming insert jobs support
s3.ingestion_mode=ONCEfor one-time, batched ingestion.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)