Skip to content

Conversation

@ibzib
Copy link

@ibzib ibzib commented Sep 18, 2019

We've gotten a lot of questions about this on the mailing list, particularly from new users, so hopefully this makes things a bit easier.

  • Use loopback for getting started; don't require a docker build.
  • Explain the differences between the different values for environment_type.
  • Remove runner-specific wordcount instructions from the portability page. The runner documentation pages should be the source of truth for those.

R: @mxm, @angoenka

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status 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
Build Status
Build Status --- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status Build Status Build Status
Portable --- Build Status --- ---

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

Copy link
Contributor

@mxm mxm left a comment

Choose a reason for hiding this comment

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

Great work! Looks good to me.

<span class="language-py">3. Submit the pipeline as above.
Note however that when you want to run your pipeline on a distributed/remote Flink cluster, you will
have to change the `environment_type` option.
See [here]({{ site.baseurl }}/roadmap/portability/#sdk-harness-config) for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should mention here that the default environment_type is DOCKER and there is no need to specify anything for environment_type if that deployment option is ok.

Copy link
Author

Choose a reason for hiding this comment

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

I changed the instructions to use loopback, so docker would be a change from that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that. I just think that it would be fair to point out the default is Docker and the LOOPBACK is just for experimentation.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I made that more explicit.

@ibzib
Copy link
Author

ibzib commented Sep 18, 2019

R: @soyrice

I saw you were working on container documentation, so I wanted to sync up on this.

Copy link
Contributor

@angoenka angoenka left a comment

Choose a reason for hiding this comment

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

Thanks!

@ibzib
Copy link
Author

ibzib commented Sep 19, 2019

Python docs failure is unrelated, should be fixed by #9620

Leaving BEAM-8273 as a follow-up.

@ibzib ibzib merged commit 6a81cdc into apache:master Sep 19, 2019
@tweise
Copy link
Contributor

tweise commented Sep 23, 2019

Thanks for working on this. I would have liked to review this PR, but did not even know it existed..

Couple issues:

  • We should probably show how to run the wordcount example on the runner page (it was removed from the portability page)
  • There is still a gradle command left. Doesn't the Flink runner wrapper eliminate the need to have that gradle build step?

@mxm
Copy link
Contributor

mxm commented Sep 23, 2019

@tweise These are great follows ups. Want to open a PR? The Gradle command is still required to build from master. For releases it is not necessary.

@tweise
Copy link
Contributor

tweise commented Sep 23, 2019

@mxm these are instructions for users. We can mention what people can do with master as well, but first we need to cover the case of a user working with a release. That is also why I think changes to these docs will benefit from more eyes.

@tweise
Copy link
Contributor

tweise commented Sep 23, 2019

@ibzib should the JIRAs be resolved? Regardless of this PR, Beam has a problem with JIRA status. Users get confused to find tickets open for which changes have been merged long ago. It is time to bring back the topic of the committer merging a PR also being responsible for resolving the ticket.

@mxm
Copy link
Contributor

mxm commented Sep 23, 2019

Yes I agree but props for Kyle's initiative to start with that. There is a balance to strike in terms of the total number of reviewers when requesting reviews from people. The current status is already much better and we can iterate further on it using your feedback.

@tweise
Copy link
Contributor

tweise commented Sep 23, 2019

@mxm there is actually no harm tagging more people on a PR. Broader review is good and it may help with faster feedback as well. Not everyone needs to respond. It is sufficient to have a single approval.

@mxm
Copy link
Contributor

mxm commented Sep 23, 2019

Slightly diverging from the topic here. I don't think it is as easy as tagging the entire set of Beam committers on every PR. Choosing the right audience for a PR is vital. You want to avoid swamping people with PRs, but you also want a good review. Tagging someone means taking time from that person and it also implies leaving some time for the person to comment, as well as replying to the comments.

@tweise
Copy link
Contributor

tweise commented Sep 23, 2019

And leaving some time is a good idea regardless who is tagged on the PR :)

When I look for reviewers for my PRs I try to pick folks that I know are most knowledgeable with the area or have expressed interest in the topic. I have never come across a case that required to tag all committers so far :)

@ibzib
Copy link
Author

ibzib commented Sep 24, 2019

@tweise I resolved the jiras. (I try to go through every once in a while and clean out the ones I forgot to close.) I'll be sure to tag you on future related PRs -- and yes, we can wait longer for review, especially for this variety of non-pressing documentation change.

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.

4 participants