Warn on suspicious Dag and task IDs at Java SDK build time#69937
Warn on suspicious Dag and task IDs at Java SDK build time#69937Andrushika wants to merge 1 commit into
Conversation
jason810496
left a comment
There was a problem hiding this comment.
Thank for the fix! I found that I need to fix the coordinator layer first (the interface to spawn the lang-sdk side subprocess) so both #69965 and this one need to be on hold for a moment.
I will share more context here soon and let you know once the coordinator side patch merged. Thanks.
|
If you use an invalid dag id, it would match nothing on the Python (stub) side. We can perhaps add some detection logic if you have a potential dag (or task) id mismatch between the stub and implementation, but honestly I’m not convinced we need to actively check for the character set. Doing this also adds the potential that the check may become stale if the allowed character set changes (it changed before). If we really want to checks this, I would want to do this in the dag processor, and only in Python (i.e. the JAR or executable needs to send the id to Python to be validated, instead of validating directly in the language sdk). |
There was a problem hiding this comment.
If you use an invalid dag id, it would match nothing on the Python (stub) side. We can perhaps add some detection logic if you have a potential dag (or task) id mismatch between the stub and implementation, but honestly I’m not convinced we need to actively check for the character set. Doing this also adds the potential that the check may become stale if the allowed character set changes (it changed before).
I see. It makes sense not to validate for the case of Mixed Lang Dag. From my perspective, the validation should be useful for the upcoming native Dag so no harm to introduce it. (Or we can on hold until the native Dag out) WDYT? Thanks.
|
Oh, I see...... I have no problem putting this on hold or even closing it. The reason I opened this PR is that I thought the behavior across lang SDKs should be aligned. I saw #69400 in ts-sdk, so I thought it would be good to introduce the same validation in java-sdk.
For the current mixed-lang Dag, I agree that the Python stub is the source of truth, so validation in each lang SDK may not be needed.
If we need validation in lang SDKs for native Dag in the future, the discrepancy TP mentioned would still exist. In that case, maybe we can keep the valid and invalid Dag/task IDs as shared test cases and let all the lang SDKs use them, instead of defining the expected behavior separately in each SDK? |
|
@Andrushika — There are 1 unresolved review thread(s) on this PR from @jason810496. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
Hard validation of the ID character set in the SDK can go stale when the server changes its allowed set, and the Airflow server already validates IDs authoritatively (see the review on apache#69937). Warn from the annotation processor at build time instead of failing at registration, so authors still get a hint while the server stays the source of truth.
c8aad4b to
50551f0
Compare
Even in this case, the validation should happen in either the dag processor or the dag submission API endpoint (and surfaced in the dag processor), for the same reasons I mentioned in the second paragraph. This should not be implemented in every language. |
|
I see some of the PRs have already been merged without this discussion being resolved. They should be reverted. |
|
Hi TP! Here's the related PR, I tried to solve the same problem in go-sdk, and Jason gave some suggestion here: #69965 (review) Jason suggests the "compile-time" best-effort validation: only shows a warning when an invalid id detected. I think this could prevent drift and preserve the source of truth while giving appropriate hints when users compile their lang-sdk stub. I've implemented in java-sdk and force-pushed in the latest commit currently: Thanks! |
Following the review here, hard validation in the SDK looks like the wrong fit. The allowed ID character set has changed before, and the Airflow server already validates it authoritatively. So instead of failing at registration, this warns at build time from the annotation processor when a
@Builder.Dagor@Builder.TaskID would be rejected by the server (longer than 250 chars, or characters outside letters, digits,-,.,_). The build still succeeds and the message points at the server as the source of truth, so it is best-effort only. Same approach as the Go SDK in #69965.related: #69400, #69965
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines