Skip to content

Conversation

@tillrohrmann
Copy link
Contributor

Rework of the existing native K8s documentation.

cc @rmetzger, @XComp, @wangyang0918

localhost_4000_deployment_resource-providers_native_kubernetes html

@flinkbot
Copy link
Collaborator

flinkbot commented Dec 3, 2020

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 4ee770e (Thu Dec 03 17:19:26 UTC 2020)

✅no warnings

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@flinkbot
Copy link
Collaborator

flinkbot commented Dec 3, 2020

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run travis re-run the last Travis build
  • @flinkbot run azure re-run the last Azure build

Copy link
Contributor

@wangyang0918 wangyang0918 left a comment

Choose a reason for hiding this comment

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

Thanks for reconstructing the native K8s documentation. I think it looks clearer than before and really good to me. I just have left some open questions.

BTW, I have run all the commands in this documentation. They work well.


Use the following command to start a PyFlink application, assuming the application image name is **my-pyflink-app:latest**.
{% highlight bash %}
$ ./bin/flink run-application -p 8 -t kubernetes-application \
Copy link
Contributor

Choose a reason for hiding this comment

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

So we will add a new page for how to submit python jobs to K8s/Yarn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need a page for describing in general how to deploy a Python program. What I am not so sure is whether we need this for every resource provider. Here, for example, there is not much specific to K8s. One needs to bundle the Python dependencies and then call flink with some special parameters which, hopefully, are the same for all different types of deployments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @dianfu @shuiqiangchen could share more information here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your clarification. At my point of view, it would be better to add a Deployment page under Python API for describing how to deploy a Python program on different cluster(standalone/Yarn/K8s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the Python deployment specific to the used resource-provider (Yarn, K8s, standalone)? Where exactly in the current structure would you put it @shuiqiangchen? Can you provide the full path (from a navigational point of view, e.g. Deployment -> Resource Providers -> Standalone -> Docker).

Copy link
Contributor

@shuiqiangchen shuiqiangchen Dec 8, 2020

Choose a reason for hiding this comment

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

No, the Python deployment does not require any specified resource-provider. So here I'm in favour of your suggestion that adding a new page to generally describe the different deployments, which is out of this PR. And the full path might be Application Development -> Python API -> Deployment, how do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, deployment under development might be a bit confusing. Maybe we can add a Deployment -> Python page and link this page from Application Development -> Python API?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I feel the same that we should illustrate development and deployment separately. I will add the Deployment -> Python page in a new PR.

RUN ln -s /usr/bin/python3 /usr/bin/python
# install Python Flink
RUN pip3 install apache-flink
Copy link
Contributor

Choose a reason for hiding this comment

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

We should install a specific version; this will always fetch the latest.

Copy link
Contributor

@shuiqiangchen shuiqiangchen Dec 7, 2020

Choose a reason for hiding this comment

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

Yes, executing the command will install the latest version of PyFlink, maybe RUN pip3 install apache-flink[==<SPECIFIC_VERSION>] is better while the [==<SPECIFIC_VERSION>] is optional.

Copy link
Contributor Author

@tillrohrmann tillrohrmann Dec 7, 2020

Choose a reason for hiding this comment

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

I would suggest to update this as part of reworking the docker.md page as my change's intention is to only maintain the current information we have. cc @rmetzger who is currently reworking these pages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for the ping. I'll update the instructions.

@tillrohrmann
Copy link
Contributor Author

Thanks a lot for the review @zentol and @wangyang0918. I have addressed most of your comments and pushed an update.

Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

Hi @tillrohrmann . Thanks for addressing the native k8s documentation. I added my comments to the English version only. The same comments apply for the Chinese version.

@tillrohrmann
Copy link
Contributor Author

Thanks for the review @XComp. I've addressed your comments and pushed an update.

Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

I followed all links of the page. No problems found. 👍

@tillrohrmann
Copy link
Contributor Author

Thanks again for the review @XComp. I've addressed your comments and squashed all fixup commits.

tillrohrmann added a commit that referenced this pull request Dec 8, 2020
Remove old native_kubernetes.md files

This closes #14305.
@tillrohrmann tillrohrmann deleted the FLINK-20355 branch December 8, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants