jason810496 commented on code in PR #69827:
URL: https://github.com/apache/airflow/pull/69827#discussion_r3840176074
##########
java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/execution/Task.kt:
##########
@@ -74,19 +77,32 @@ internal object TaskRunner {
val definition =
bundle.dags[request.ti.dagId]?.tasks[request.ti.taskId]?.definition
?: return TaskResult.of(TaskState.State.REMOVED)
+ val instance =
+ try {
+ definition.getDeclaredConstructor().newInstance()
+ } catch (e: InvocationTargetException) {
+ val cause = e.cause ?: e
+ logger.error(
+ "Task class constructor threw an exception",
+ mapOf("ti" to request.ti, "taskClass" to definition.name, "error" to
cause, "trace" to cause.stackTraceToString()),
+ )
+ // Retrying cannot help: instantiation fails the same way on every try.
+ return TaskResult.failure(shouldRetry = false)
+ } catch (e: Throwable) {
+ logger.error(
+ "Cannot instantiate task class. A task class must be concrete and
declare a public no-argument constructor",
+ mapOf("ti" to request.ti, "taskClass" to definition.name, "error" to
e, "trace" to e.stackTraceToString()),
+ )
+ return TaskResult.failure(shouldRetry = false)
Review Comment:
`Throwable` also catches static-initializer and linkage failures from
otherwise valid classes, so this reports the wrong contract error. Narrow the
structural branch and keep initialization failures distinct.
```suggestion
} catch (e: ReflectiveOperationException) {
logger.error(
"Cannot instantiate task class. A task class must be public and
concrete and declare a public no-argument constructor",
mapOf("ti" to request.ti, "taskClass" to definition.name, "error"
to e, "trace" to e.stackTraceToString()),
)
return TaskResult.failure(shouldRetry = false)
} catch (e: Throwable) {
logger.error(
"Error initializing task class",
mapOf("ti" to request.ti, "taskClass" to definition.name, "error"
to e, "trace" to e.stackTraceToString()),
)
return TaskResult.failure(request.tiContext.shouldRetry)
}
```
##########
dev/breeze/src/airflow_breeze/utils/selective_checks.py:
##########
@@ -245,6 +245,7 @@ def __hash__(self):
FileGroupForCi.JAVA_SDK_E2E_FILES: [
# `.md` excluded — doc-only edits do not affect the Gradle build.
r"^java-sdk/(?!.*\.md$).*",
+ r"^airflow-e2e-tests/java-test-bundle/.*",
Review Comment:
This match makes fixture-only changes run the Java SDK E2E tests and build
the PROD image. Please keep the exhaustive decision rules in sync.
Suggested text for `04_selective_checks.md`:
```markdown
* `Java SDK E2E tests` run when Java SDK sources, the Java test bundle, the
Java E2E suite or Docker files, or Java coordinator sources change. Enabling
them also forces `PROD Image building`.
```
##########
java-sdk/processor/src/test/kotlin/org/apache/airflow/sdk/BuilderTest.kt:
##########
@@ -368,4 +368,24 @@ class BuilderTest {
"Cannot create task from vararg function t1",
)
}
+
+ @Test
+ @DisplayName("fail compilation when dag class is a non-static inner class")
+ fun failCompilationWhenDagClassIsNonStaticInner() {
+ val compilation =
+ compile(
+ """
+ package org.apache.airflow.example;
+ import org.apache.airflow.sdk.Builder;
+ public class TestExample {
+ @Builder.Dag
+ public class InnerDag { @Builder.Task(id = "foo") public void t1()
{} }
+ }
+ """,
+ )
+ assertThat(compilation).failed()
+ assertThat(compilation).hadErrorContaining(
+ "an enclosing instance that contains
org.apache.airflow.example.TestExample.InnerDag is required",
+ )
+ }
Review Comment:
This only asserts a pre-existing annotation-processor failure and still
passes without the runtime fix, so it is unrelated coverage.
```suggestion
```
##########
airflow-core/docs/authoring-and-scheduling/language-sdks/java.rst:
##########
@@ -205,6 +205,16 @@ Interface-based API
Implement the ``Task`` interface directly for full control over how tasks are
registered and how XComs are
read. Each task is registered as a ``TaskDef`` on a ``DagDef``.
+The runner creates a fresh instance of the task class through reflection for
every task-instance run,
+which puts three constraints on the class:
+
+* It must be concrete: not abstract and not an interface.
+* It must declare a public no-argument constructor.
+* If nested inside another class, it must be a ``static`` nested class.
+
+A class that violates any of these fails at runtime with a ``Cannot
instantiate task class`` error in the
+task log.
Review Comment:
Package-private task classes still fail with `IllegalAccessException`. Add
class visibility to the constraints and mirror `public` in the runtime error.
```suggestion
The runner creates a fresh instance of the task class through reflection for
every task-instance run,
which puts four constraints on the class:
* The task class itself must be ``public``.
* It must be concrete: not abstract and not an interface.
* It must declare a public no-argument constructor.
* If nested inside another class, it must be a ``static`` nested class.
A class that violates any of these fails at runtime with a ``Cannot
instantiate task class`` error in the
task log.
```
--
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]
