You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@flink.apache.org by GitBox <gi...@apache.org> on 2022/03/16 18:20:04 UTC
[GitHub] [flink-kubernetes-operator] gyfora opened a new pull request #75: [FLINK-26640] Make FlinkVersion enum and required
gyfora opened a new pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75
This PR changes the FlinkVersion from string to enum.
I simply copied the FlinkVersion enum from flink master so that we can adapt this directly once we bump the flink dep to at least 1.15.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1069906445
> I am wondering if we should require the user to explicitly set the flink version or provide a default value like I did now
It is not very easy to predict the flinkVersion if a user customized image is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 commented on a change in pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#discussion_r828689234
##########
File path: helm/flink-operator/crds/flinkdeployments.flink.apache.org-v1.yml
##########
@@ -26,6 +26,22 @@ spec:
serviceAccount:
type: string
flinkVersion:
+ enum:
+ - v1_3
Review comment:
Having our own enum of FlinkVersion also makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] tweise commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
tweise commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1072778332
@gyfora I just realized that my comment wasn't posted.. I'm actually not sure if we want to restrict the upper bound of the Flink version. I imagine that in the long run the underlying Flink dependencies will converge towards a good compatibility story and then only the minimum version matters. When no version is specified, we assume the minimum version. When a higher version is specified, features of that higher version can be used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] tweise commented on a change in pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
tweise commented on a change in pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#discussion_r828299762
##########
File path: helm/flink-operator/crds/flinkdeployments.flink.apache.org-v1.yml
##########
@@ -26,6 +26,22 @@ spec:
serviceAccount:
type: string
flinkVersion:
+ enum:
+ - v1_3
Review comment:
Do we really want those old (irrelevant) Flink versions here? It might be better to only list the versions that are actually supported. That probably means that we need to have our own enum. And that should be fine, since the validation can still check it is one of `FlinkVersion`. That would also avoid having to copy the entire `FlinkVersion` class, we can add that check when 1.15 becomes the new minimum version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora merged pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora merged pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1070540894
> > I am wondering if we should require the user to explicitly set the flink version or provide a default value like I did now
>
> It is not very easy to predict the flinkVersion if a user customized image is used.
@wangyang0918 are you suggesting that it should be explicitly required always in the yaml?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1070544686
> are you suggesting that it should be explicitly required always in the yaml?
For now, I would say yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1070576954
Sounds good, we can relax this later. I will update the PR (+examples, docs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1069370030
cc @tweise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on a change in pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
morhidi commented on a change in pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#discussion_r828306476
##########
File path: helm/flink-operator/crds/flinkdeployments.flink.apache.org-v1.yml
##########
@@ -26,6 +26,22 @@ spec:
serviceAccount:
type: string
flinkVersion:
+ enum:
+ - v1_3
Review comment:
makes perfect sense to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1069585554
I am wondering if we should require the user to explicitly set the flink version or provide a default value like I did now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on a change in pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#discussion_r828302232
##########
File path: helm/flink-operator/crds/flinkdeployments.flink.apache.org-v1.yml
##########
@@ -26,6 +26,22 @@ spec:
serviceAccount:
type: string
flinkVersion:
+ enum:
+ - v1_3
Review comment:
I don't have a problem with having our own enum either (that way we would have immediate validation for the accepted versions). @wangyang0918 , @morhidi what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1069584409
Updated the PR @tweise please take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] tweise commented on pull request #75: [FLINK-26640] Make FlinkVersion enum and required
Posted by GitBox <gi...@apache.org>.
tweise commented on pull request #75:
URL: https://github.com/apache/flink-kubernetes-operator/pull/75#issuecomment-1072778736
I also think it would be nice to have a way to infer the version when the official docker image is used!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org