guan404ming commented on code in PR #71144:
URL: https://github.com/apache/airflow/pull/71144#discussion_r3759192285
##########
ts-sdk/src/coordinator/runtime.ts:
##########
@@ -51,13 +52,46 @@ import {
type RuntimeTaskState,
type StartupDetails,
} from "./protocol.js";
-import { getRegisteredTask, listRegisteredTasks } from "../sdk/registry.js";
+import { DagRegistry } from "../sdk/registry.js";
import type { TaskContext, TaskHandlerArgs } from "../sdk/task.js";
import type { JsonValue } from "../sdk/client-types.js";
export const ABORT_GRACE_PERIOD_MS = 30_000;
export const COORDINATOR_RESPONSE_TIMEOUT_MS = 30_000;
+let served = false;
Review Comment:
Nice cleanup overall! I think there are two small leftovers worth a look:
1. served is set before `startCoordinator` runs and never cleared on
failure, so a rejected serveDags (e.g. Missing --comm) blocks retries with a
misleading "already called" -> public-api.test.ts actually relies on this
sequence. Moving the state onto the registry would fix cleanup and test
isolation.
2. The instanceof DagRegistry / instanceof Dag checks reject genuine objects
when two SDK copies are installed (dual-package hazard). A structural check
would avoid that.
Nit: `serveDags` throws sync for these but rejects for runtime errors; async
would unify the channels.
##########
ts-sdk/src/cli/pack.ts:
##########
Review Comment:
One nit consistency thought: pack trusts the manifest more than the SDK
layer trusts TypeScript, no suspicious-id warnings (go-pack has
`warnOnSuspiciousIDs`), `supervisor_schema_version` accepts any truthy value,
and top-level null crashes with a raw `TypeError`. Failing closed here for
everything entering the manifest might be worth it.
--
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]
