github-actions[bot] commented on code in PR #66914:
URL: https://github.com/apache/doris/pull/66914#discussion_r3837372474
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java:
##########
@@ -1335,7 +1399,16 @@ public void rollback() {
@Override
public void close() {
- // No resources to release: the SDK transaction holds no connections
of its own.
+ IcebergStatementScope.TrackedTableLease lease;
+ synchronized (beginLock) {
+ closed = true;
+ lease = tableLease;
+ tableLease = null;
+ beginLock.notifyAll();
+ }
+ if (lease != null) {
+ lease.close();
Review Comment:
[P1] Keep this lease until every admitted rewrite group has quiesced.
runGroups() returns on timeout or interruption without canceling or joining its
submitted tasks, and the caller immediately rolls back/removes the shared
transaction and closes this lease. The beginLock fixes a first worker still
publishing the table, but a later worker can already have observed writeStarted
and continue planning/executing against transaction.getTable() after this
close, then report fragments for a removed transaction. This is distinct from
the existing first-begin race. Please cancel and await all groups before
rollback, or otherwise retain per-group ownership until the active-group count
reaches zero, with a latch test for a second worker paused after the begin-once
gate.
##########
fe/fe-core/src/main/java/org/apache/doris/service/arrowflight/sessions/FlightSqlConnectPoolMgr.java:
##########
@@ -57,6 +59,12 @@ public int registerConnection(ConnectContext ctx) {
@Override
public void unregisterConnection(ConnectContext ctx) {
+ // Reject new publications first, then signal the active query before
waiting for an admitted
+ // GetFlightInfo publisher. Waiting before cancellation can deadlock
KILL CONNECTION behind the
+ // publisher whose query must be canceled in order to leave
publication.
+ ctx.sealFlightSqlDeferredExecutors();
+ ctx.cancelQuery(new Status(TStatusCode.CANCELLED, "arrow flight
connection closed"));
Review Comment:
[P1] Preserve cancellation across executor publication. A GetFlightInfo
request is counted as a publisher before ConnectProcessor reaches
ctx.setExecutor(). If CloseSession or token eviction seals here during that
gap, cancelQuery() sees null, the request can then install a blocking executor,
and awaitAndCloseFlightSqlDeferredExecutors() waits forever because
setExecutor() does not observe the seal or replay the cancel. This is distinct
from the existing cancel-before-wait thread, which covers an executor that was
already published. Please linearize executor installation with the terminal
Flight state (or retain/replay the terminal cancel), and add a latch test that
tears down immediately before ctx.setExecutor().
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/insert/streaming/StreamingInsertTask.java:
##########
@@ -173,24 +175,66 @@ protected void onFail(String errMsg) throws JobException {
@Override
public void cancel(boolean needWaitCancelComplete) {
super.cancel(needWaitCancelComplete);
- if (null != stmtExecutor) {
+ StmtExecutor executor = stmtExecutor;
+ if (null != executor) {
log.info("cancelling streaming insert task, job id is {}, task id
is {}",
getJobId(), getTaskId());
- stmtExecutor.cancel(new Status(TStatusCode.CANCELLED, "streaming
insert task cancelled"),
- needWaitCancelComplete);
+ executor.cancel(new Status(TStatusCode.CANCELLED, "streaming
insert task cancelled"), false);
+ }
+ if (needWaitCancelComplete) {
+ // Planning may still be blocked before stmtExecutor is published.
Do not let PAUSE wait
+ // forever; the scheduler owner remains responsible for exact-once
cleanup in execute().
+ awaitExecutionCompletion(CANCEL_WAIT_TIMEOUT_MS);
Review Comment:
[P1] Do not admit a successor while this bounded wait can leave the
predecessor inside its transaction. After one second PAUSE returns, RESUME
moves the job to PENDING and createStreamingTask() overwrites
runningStreamTask. A late predecessor beforeCommitted() then reads that mutable
successor field for cancellation, task ID, offsets, backends, and load
statistics, so the old transaction can be rejected against—or attach state
from—the new task. This is distinct from the earlier null-detachment cleanup
thread. Please fence successor creation on exact predecessor completion or bind
callbacks to the initiating task identity, and add a latch test that resumes
while the predecessor is still unwinding.
##########
fe/fe-core/src/main/java/org/apache/doris/job/manager/JobManager.java:
##########
@@ -280,10 +280,10 @@ public void alterJobStatus(String jobName, JobStatus
jobStatus, FailureReason re
if (a.getJobName().equals(jobName)) {
try {
checkSameStatus(a, jobStatus);
- alterJobStatus(a.getJobId(), jobStatus);
if (a instanceof StreamingInsertJob) {
((StreamingInsertJob)
a).onManualStatusAltered(jobStatus, reason);
}
+ alterJobStatus(a.getJobId(), jobStatus);
Review Comment:
[P1] Make this a single validated manual-transition operation. The new
ordering has two failures: checkSameStatus() lets invalid
STOPPED->RUNNING/PAUSED/PENDING requests mutate failure/retry/reader state
before updateJobStatus() rejects them; and even a valid RUNNING->PAUSED
releases the job lock between onManualStatusAltered() and this call, so task
success can clear the manual reason, set needRebuildReader=false, install a
successor, or mark FINISHED before PAUSE cancels/regresses that newer state.
Please validate and publish reason plus status/cancellation under one job-lock
acquisition, do the blocking reader release/wait afterward, and add
invalid-transition plus task-success/end-of-source latch tests.
--
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]
