[BEAM-115] Add control of PipelineVisitor recursion into composite transforms #217
Conversation
|
R: @bjchambers for Dataflow bits. R: @amitsela this is consistent with intended behavior, yes? I would normally trust that doing this wrong would result in catastrophic test failures, but the odd success of tests for #214 makes me want to be extra careful. R: @mxm touches the Flink slightly, though I didn't go all the way yet. Do you agree that if I do the same thing to the Flink runner that I did to the Spark runner it will mirror the intent? CC: @tgroh non-committer who has been using this stuff most heavily And of course, feedback welcome from anyone on any bit. |
| * Control enum for indicating whether or not a traversal should process the contents of | ||
| * a composite transform or not. | ||
| */ | ||
| public enum Recurse { |
There was a problem hiding this comment.
Calling this Recurse seems weird. Maybe CompositeDisposition? CompositeBehavior? ShouldRecurse? RecursionBehavior? ...
There was a problem hiding this comment.
Yea, I figured you might have a good choice. I like ShouldRecurse but I'm flexible.
There was a problem hiding this comment.
OK, since part of the benefit of an enum over a boolean - aside from readability - is to allow expansion to non-binary decisions, it should be something more like CompositeBehavior or some such.
|
I will take a deeper look (at #214 as well) but it might take me a couple of days.. |
Adjusting in the same way you started with the Spark runner should work. I think we have two cases for composite transforms:
Otherwise, we just visit the primitive transforms that the composite transform is composed of. |
|
+1 from me |
kennknowles
force-pushed
the
pipeline-recurse
branch
2 times, most recently
from
f1b9fea to
1582ab2
Compare
|
OK, I've done the same for the Flink runner. Please take a look. |
| return CompositeBehavior.ENTER_TRANSFORM; | ||
| } | ||
| applyBatchTransform(transform, node, translator); | ||
| LOG.info(genSpaces(this.depth) + "leaveCompositeTranform-" + formatNodeName(node)); |
There was a problem hiding this comment.
leaveCompositeTransform is not called when you don't enter a composite transform, right?
There was a problem hiding this comment.
True. I wasn't sure what kind of message you might like to see here so I just made the begin/end symmetrical. Maybe something like "(not recursing) enterCompositeTransform" or whatever would work. But then the format of the string made me think maybe you wanted it to be machine parseable.
I am happy to do anything with these log messages and the depth.
There was a problem hiding this comment.
I changed the log to maybe make more sense.
kennknowles
force-pushed
the
pipeline-recurse
branch
from
1582ab2 to
83446cc
Compare
|
LGTM |
kennknowles
force-pushed
the
pipeline-recurse
branch
from
648f26c to
dbf7a06
Compare
[euphoria-core] Publish test jar.
Update `pbs_for_create`, `pbs_for_set_no_merge`, `pbs_for_set_with_merge`, and `pbs_for_update` to match semantics expected by current versions of [conformance tests](googleapis/conformance-tests@0bb8520): - Rather than create separate `Write.transform` messages to hold field transforms, inline them as `update_transforms` in the main `Write.update` message (which will always be created now). Copy in the current version of the conftest JSON files and verify. Closes apache#217
5 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.
This is to easily support wholesale replacement of composite transforms. The Flink and Spark runner already achieve this by tracking whether they are in a region of the Pipeline that should be ignored. I ported over the Spark runner to simplify it, but not the Flink runner, to get this PR out for feedback.