Skip to content

Conversation

@tgroh
Copy link
Member

@tgroh tgroh commented Apr 26, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

This allows the executor to be ignorant of the mapping from PValue to
Consumers, as well as allowing the TransformExecutor to pass bundles
that should only be consumed by specific PTransforms. This can occur if
a transform is incapable of processing a bundle.

@tgroh
Copy link
Member Author

tgroh commented Apr 26, 2016

R: @kennknowles

This is to ensure that elements don't get duplicated if, as part of #220 (and additional follow-up) a PTransform fails to process an element (due to some input not being ready) - it must schedule that element to be processed, but should not schedule it for any other consumer.

@tgroh tgroh force-pushed the ippr_add_consumers_of_completed_transforms branch 3 times, most recently from 3d07dfe to ee7e8b5 Compare April 27, 2016 17:16
*/
private static class ExecutorUpdate {
private final Optional<? extends CommittedBundle<?>> bundle;
private final Optional<? extends AppliedPTransform<?, ?, ?>> consumer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment here about the invariant that this is either/or where two things are null together.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

This allows the executor to be ignorant of the mapping from PValue to
Consumers, as well as allowing the TransformExecutor to pass bundles
that should only be consumed by specific PTransforms. This can occur if
a transform is incapable of processing a bundle.
@tgroh tgroh force-pushed the ippr_add_consumers_of_completed_transforms branch from ee7e8b5 to ad381f7 Compare April 27, 2016 21:22
@tgroh tgroh mentioned this pull request Apr 28, 2016
4 tasks
// filter them out
if (!Iterables.isEmpty(committed.getElements())) {
completed.add(committed);
outputs.put(committed, valueToConsumers.get(committed.getPCollection()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since committed carries with it all the information needed for a lookup in valueToConsumers, perhaps you don't need to convert the iterable to a map so early? Can it be deferred just to the moment you add them as pending?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes in the immediate follow-up to this CL, which allows a TransformEvaluator to return some elements as "unprocessed". Those elements are added to the result map here, but should only be consumed by the producing transform

Ex: 7da8e1a

@tgroh tgroh mentioned this pull request Apr 28, 2016
4 tasks
@kennknowles
Copy link
Member

Your intention is to resolve conflicts and overlay this on the new CommittedResult POJO, yes? If it isn't tremendously larger, feel free to combine this with an actual use of the new capability.

@tgroh
Copy link
Member Author

tgroh commented Apr 29, 2016

This can be closed with the merge of #260, which replaces it.

@tgroh tgroh closed this Apr 29, 2016
iemejia added a commit to iemejia/beam that referenced this pull request Jan 12, 2018
mareksimunek pushed a commit to mareksimunek/beam that referenced this pull request May 9, 2018
…-guava

[thirdparty-guava] use shaded jar with shadow classifier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants