-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug]: upload_graph option does not work with Dataflow Java Runner v2 #32159
Comments
I think #30604 likely caused this. https://github.com/kennknowles/beam/blob/0a0186de5552dddd21a41fc851c805bf6b3b6714/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowPipelineTranslator.java#L431 is the only runner v1/v2 logic on this path AFAIK, it only reproes on v2, and I confirmed it does work on 2.55.1 (before this change) |
Also reproduced on 2.56.0 (example failures) - another datapoint towards #30604 being the thing that exposed this FYI @kennknowles - looks like we missed an assumption. I think the easy fix may be to just do a null check on Line 1404 in edf4c4f
clear() , but I need to test it.
|
Yea, the most internally-consistent thing to do is to ignore or disable Just to state it for posterity, though you probably already know:
When I say they may be mutually exclusive, I think there is a chance that some piece of code somewhere looks in the same variable for the uploaded file, since the mechanism looks the same if you aren't type-conscious. |
Agree with @kennknowles. We can just error it out when |
I don't think we should error out, but we can warn. It doesn't make sense to me to fail the job. We also need to not automatically opt users in for runner v2 |
You can warn but that option perhaps should be removed in case some problematic code paths can be triggered when the option is there. |
Mostly, I don't want to fail the job because it harms our ability to auto-upgrade users to runner v2. Removing the option seems reasonable. |
The automatic opt in happens on the service side so I don't think it will trigger this. But I also agree that making it do nothing is probably better than failing it, because if people have command lines assembled (either manually or automatically) and they add |
Didn't someone add automatic |
Actually, this is on the SDK side ( Line 1387 in b2d26b6
Yes, but I assume folks still have it set manually |
I mean that automatic opt in to v2 is on the service side. For jobs that do not specify v1 or v2, we submit the necessary information for both and the service decides which is which. Jobs like that won't trigger the problem. This bug doesn't impact the involuntary migration to v2. It only impact jobs that explicitly choose both v2 and upload_graph prior to submission. |
Oh got it, that's true |
What happened?
Today, if you try to run a runner v2 job with the
upload_graph
option, the job fails with the following exception:I reproed this against our test suites:
Change I made - 70650bd
Result - https://github.com/apache/beam/actions/runs/10358654870
Issue Priority
Priority: 2 (default / most bugs should be filed as P2)
Issue Components
The text was updated successfully, but these errors were encountered: