[BEAM-1928] Translate PTransforms to and from Runner API Protos#2540
[BEAM-1928] Translate PTransforms to and from Runner API Protos#2540tgroh wants to merge 5 commits into
Conversation
Update SdkComponents to handle the translations.
|
Refer to this link for build results (access rights to CI server needed): |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): Failed Tests: 2beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-core-construction-java: 2
--none-- |
ParDo and Combine additional inputs should have the PCollections that are input to their PCollectionViews, not the PCollectionViews.
|
Refer to this link for build results (access rights to CI server needed): |
| import org.apache.beam.sdk.values.PValue; | ||
| import org.apache.beam.sdk.values.TupleTag; | ||
|
|
||
| /** Created by tgroh on 4/7/17. */ |
| * <p>Does not register {@code application} within the provided {@link SdkComponents}. | ||
| */ | ||
| public static RunnerApi.PTransform toProto( | ||
| AppliedPTransform<?, ?, ?> application, |
There was a problem hiding this comment.
application -> appliedPTransform
This will also make the comment easier to read
| // TODO: Display Data | ||
|
|
||
| PTransform<?, ?> transform = application.getTransform(); | ||
| if (KNOWN_PAYLOAD_TRANSLATORS.containsKey(transform.getClass())) { |
There was a problem hiding this comment.
Using a service loader for known payload translators will help for user defined composites which want to bind transform to/from proto using the data block
There was a problem hiding this comment.
ACK. That is the goal, though potentially in addition to the constant set of known payloads, which will appear in a later change.
| * | ||
| * <p>Does not register {@code application} within the provided {@link SdkComponents}. | ||
| */ | ||
| public static RunnerApi.PTransform toProto( |
| * will not be added to the components produced by this {@link SdkComponents} until it is | ||
| * translated via {@link #registerPTransform(AppliedPTransform, List)}. | ||
| */ | ||
| public String registerPTransform(AppliedPTransform<?, ?, ?> pTransform) { |
There was a problem hiding this comment.
private?
Seems like nobody should call this method as its usage is an internal detail of this class
There was a problem hiding this comment.
package-private. It's required when converting a PTransform, as we should not forbid the translation of a composite before any of its children.
| } | ||
| checkState( | ||
| missingTransforms.isEmpty(), | ||
| "%s transforms were registered but were never added to components. Missing transform %s", |
There was a problem hiding this comment.
Is there a way to get it so that registerPTransform(AppliedPTransform, List) is always invoked (with an empty list representing no children) instead of needing this validate method?
There was a problem hiding this comment.
We don't keep information about child nodes within AppliedPTransform, so I don't believe it is at the current time.
tgroh
force-pushed
the
ptransforms_utilities
branch
from
f0b225b to
1c8472b
Compare
|
LGTM |
4 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.
Update SdkComponents to handle the translations.