[BEAM-310] Refactor BundleFactory methods #1019
Conversation
Remove the inputBundle parameter to createBundle and createKeyedBundle. This parameter is unless the model is capable of propagating keys between PTransforms. Remove the PCollection parameter to createRootBundle. createRootBundle should be a par Make [Un]CommmittedBundle#getPCollection nullable. Bundles are utilized by the runner to control the processing of elements, but may not always belong to a PCollection. Update ImmutabilityCheckingBundleFactory to return an underlying bundle as the result of createRootBundle. Use createBundle in Root transform evaluators.
tgroh
changed the title
|
R: @lukecwik This is an underlying change to modify how root transforms are exercised in order to support splitting. |
| /** | ||
| * Returns the PCollection that the elements of this {@link UncommittedBundle} belong to. | ||
| */ | ||
| @Nullable |
There was a problem hiding this comment.
It would be nice if these bundles weren't associated with the pcollection cause having nulls here will need to have people understand the special case of the bundle being the input to a root transform. Ditto on CommittedBundle
There was a problem hiding this comment.
Agreed. Filed https://issues.apache.org/jira/browse/BEAM-689
| abstract class StructuralKey<K> { | ||
|
|
||
| private StructuralKey() { | ||
| // Do not extend outside of this class |
There was a problem hiding this comment.
You can prevent people extending the class by requiring a class to be given to the constructor which has private class visibility and is final.
There was a problem hiding this comment.
That's what this is; Updated comment
| } | ||
|
|
||
| public abstract K getKey(); | ||
| /** |
There was a problem hiding this comment.
add new line to separate getKey
|
|
||
| public abstract K getKey(); | ||
| /** | ||
| * Get the empty {@link StructuralKey}. All instances of the empty key are considered equal. |
There was a problem hiding this comment.
I was under the impression that all instances of the empty key should be considered distinct
| if (other == this) { | ||
| return true; | ||
| @Override | ||
| public K getKey() { |
There was a problem hiding this comment.
Do we need a unique instance of the key to be decoded each time getKey is returned?
Is this to prevent mutations of K?
There was a problem hiding this comment.
It means that the runner only ever provides keys that have been deserialized from the initial representation it received. We only use it to deliver timers, but it means that timer deliveries are independent from the original key (as is appropriate).
| public void emptyKeysNotEqual() { | ||
| StructuralKey<?> empty = StructuralKey.empty(); | ||
|
|
||
| assertThat(empty, not(Matchers.<StructuralKey<?>>equalTo(StructuralKey.empty()))); |
There was a problem hiding this comment.
Looks good but overly verbose when compared to assertEquals and assertNotEquals found in org.junit.Assert
There was a problem hiding this comment.
Yes, but this is the general pattern used in (almost) all of the other DirectRunner tests.
|
Minor comment and then LGTM |
|
Closed in 759b6ca |
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.
Remove the inputBundle parameter to createBundle and createKeyedBundle.
This parameter is unless the model is capable of propagating keys
between PTransforms.
Remove the PCollection parameter to createRootBundle. createRootBundle
should be a par
Make [Un]CommmittedBundle#getPCollection nullable. Bundles are utilized
by the runner to control the processing of elements, but may not always
belong to a PCollection. Update ImmutabilityCheckingBundleFactory to
return an underlying bundle as the result of createRootBundle.
Use createBundle in Root transform evaluators.