jason810496 commented on code in PR #74690: URL: https://github.com/apache/airflow/pull/74690#discussion_r4323132929
########## java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/Context.kt: ########## @@ -127,10 +129,49 @@ data class Context( /** Registration of the executing task; resolves wired data inputs. */ internal var taskDef: TaskDef? = null + internal var cancellation = Cancellation() Review Comment: How about constructing the `Cancellation()` once instead of constructing in several places? Here're more context that I ask agent to double check: --- Only the instance in `runTaskAndReport` (Server.kt) is connected to the signal trap. The others are defaults: this field, `Context.from` (line 174), `TaskRunner.runTask` (Task.kt line 101, never used since both wrappers pass it), and `runTask(bundle, request, client)` (Task.kt line 170, only `TaskTest` relies on it). If a future caller forgets to pass the server's instance, it gets an unconnected one with no error. `isCancelled()` then marks the wrong instance as watched, and SIGTERM goes to the JVM. Could we create it once in `runTaskAndReport` and make the parameter required in `Context.from` and both `runTask` functions? Tests can pass `Cancellation()` explicitly. This field's initializer also allocates an instance that `Context.from` replaces right away. ########## java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/Context.kt: ########## @@ -127,10 +129,49 @@ data class Context( /** Registration of the executing task; resolves wired data inputs. */ internal var taskDef: TaskDef? = null + internal var cancellation = Cancellation() + + /** + * Whether Airflow has asked this task to stop. + * + * Airflow asks a running task to stop when, for example, a user marks its task + * instance as failed. The supervisor, which is the Airflow process that started + * the JVM, then sends the JVM SIGTERM. If the JVM is still running after a + * grace period, the supervisor sends SIGKILL. + * + * Once a task has called this method or [awaitCancellation], the SDK catches + * the first SIGTERM or SIGINT. The SDK records the stop request instead of + * letting the JVM exit, so the task can stop its work before SIGKILL arrives. + * From then on the task must keep checking until it returns. + * + * If a task has called neither method, or if a second signal arrives, the JVM + * handles the signal as it would without the SDK. By default the JVM runs its + * shutdown hooks and exits. + * + * A context made with [copy] does not see the stop request. Review Comment: Would it better to block the `copy` method at all or the share the same `Cancellation` instance for the `Context`? --- Since is a public data class, `copy()` is easy to reach for, and the copy gets its own `Cancellation` that the signal trap never sees. If a task checks only through the copy, the real instance is never marked as watched, so SIGTERM goes to the JVM. Is documenting it enough, or should the copy share the original instance? -- 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]
