Skip to content
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

[BEAM-6740] Add comment explaining unused combine.globally translation #7948

Closed

Conversation

@echauchot
Copy link
Contributor

echauchot commented Feb 26, 2019

Please add a meaningful description for your change here


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Python Build Status
Build Status
--- Build Status
Build Status
Build Status --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@echauchot echauchot requested a review from kennknowles Feb 26, 2019
Copy link
Member

kennknowles left a comment

It might be pretty easy to follow the example at

to create the translator.

@echauchot

This comment has been minimized.

Copy link
Contributor Author

echauchot commented Feb 27, 2019

It might be pretty easy to follow the example at

beam/runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/CombineTranslation.java

Line 56 in b83b302

public static class CombinePayloadTranslator
to create the translator.

Thanks Kenn for taking a look.
Sure it is just that I don't have time right now to create one and deal with the impacts and tests for all the runners. So I prefer to focus on making combine work on the new POC runner even if I temporary get back to translator resolution based on classes. I leave this PR open in case I have time to add the translator later on

@stale

This comment has been minimized.

Copy link

stale bot commented Apr 28, 2019

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Apr 28, 2019
@echauchot

This comment has been minimized.

Copy link
Contributor Author

echauchot commented Apr 29, 2019

closing because the fix is ongoing

@echauchot echauchot closed this Apr 29, 2019
@echauchot echauchot deleted the echauchot:BEAM-6740-combine-globally-spark branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.