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

[FLINK-16985] Stateful Functions Examples have their specific job names #106

Closed
wants to merge 1 commit into from

Conversation

UnityLung
Copy link
Contributor

@UnityLung UnityLung commented May 7, 2020

Stateful Functions Examples add their specific job names.

Greeter-example
Python-greeter-example
Python-kubernetes-example
Ridesharing-example
Shopping-cart-example

@UnityLung
Copy link
Contributor Author

@tzulitai Can you please review it?

@UnityLung UnityLung changed the title Stateful Functions Examples have their specific job names [FLINK-16985] Stateful Functions Examples have their specific job names May 8, 2020
@UnityLung UnityLung closed this May 8, 2020
@UnityLung UnityLung reopened this May 8, 2020
@igalshilman
Copy link
Contributor

Hi there,
Thanks a lot for opening your first PR for this project, and welcome :-)

I think that everything we will put in the examples folks would accidentally think that this is mandatory and copy to their projects.
I’d really like the Dockerfile to stay as simple as possible at this moment, to lower the entrance barrier for StateFun.

Did you find the lack of a specific job name confusing for you?

@tzulitai
Copy link
Contributor

I originally opened FLINK-16985.
I wouldn't say lacking the specific job name is confusing, but maybe rounds up a better user experience for new users who are just coming to this repo for the first time, running the application, and then looking at the Flink Web UI to see what's happening behind the scenes.

But I agree that adding this extra bit to the example Dockerfiles can be a bit distracting from a "introductory examples" point of view.

@igalshilman
Copy link
Contributor

igalshilman commented May 14, 2020

Hi @abc863377, I think that there is a consensus not to proceed with the change here, but I'd like to suggest the following: can you set the statefun.flink-job-name key at tools/docker/flink-distribution-template/conf/flink-conf.yaml so that people who would like to change the job name, would know what to modify.

The suggest change is to add:

statefun.flink-job-name: Statefun Application

@UnityLung
Copy link
Contributor Author

Hi @igalshilman ,
Thanks for the suggestion. I'll follow your advice and keep Dockerfile simple.

For someone who want to change the job name.

flink-statefun/tools/docker/flink-distribution-template/conf/flink-conf.yaml

statefun.flink-job-name: Statefun Application
Copy link
Contributor

@igalshilman igalshilman left a comment

Choose a reason for hiding this comment

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

Hi @abc863377,
Thanks for adding the statefun.flink-job-name configuration to the distribution template, this sets a good example!
However I thought we reached an agreement to not changing the Dockerfiles? can you please revert the change in the greeter example?
Other then that, I'm good with merging this!

@UnityLung
Copy link
Contributor Author

Hi, @igalshilman ,
Sorry. I am a Git beginner.

Now I removed the old commit. and updated the new statefun.flink-job-name configuration to the distribution template.

Can you please review it?

Thanks a lot.

@igalshilman
Copy link
Contributor

Hi @abc863377,
No worries, thanks a lot for your first contribution to this project!

@sjwiesman
Copy link
Contributor

Merging . . .

@sjwiesman sjwiesman closed this in c04f111 May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants