You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/19 13:43:51 UTC
[GitHub] [flink-kubernetes-operator] bgeng777 opened a new pull request, #173: [FLINK-27310] Fix FlinkOperatorITCase
bgeng777 opened a new pull request, #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173
Fix FlinkOperatorITCase that not sets JM replica correctly.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 merged pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 merged PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853706739
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/FlinkOperatorITCase.java:
##########
@@ -114,6 +114,7 @@ private static FlinkDeployment buildSessionCluster() {
resource.setCpu(1);
JobManagerSpec jm = new JobManagerSpec();
jm.setResource(resource);
+ jm.setReplicas(1);
Review Comment:
Yes. I believe the `replicas` is not a field which should be configured explicitly. In most cases, no matter the HA enabled or not, they do not need to set this. Unless they really want a standby JobManager to get faster recovery.
Maybe we could also remove the `replicas` in the examples and `e2e-tests`.
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853740004
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
Could you please share a failed CI when using `mvn verify`?
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853808587
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
cd ..
- name: Tests in flink-kubernetes-webhook
Review Comment:
Could we also use the `mvn verify` in "Tests in flink-kubernetes-webhook" even though we do not have ITCase now?
##########
.gitignore:
##########
@@ -35,3 +35,4 @@ buildNumber.properties
.idea
*.iml
+*.DS_Store
Review Comment:
nit: Keep the empty line at end of file.
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853810148
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/FlinkOperatorITCase.java:
##########
@@ -114,6 +114,7 @@ private static FlinkDeployment buildSessionCluster() {
resource.setCpu(1);
JobManagerSpec jm = new JobManagerSpec();
jm.setResource(resource);
+ jm.setReplicas(1);
Review Comment:
Would you like to change the default value of `replicas` in this PR or it will be done in a follow-up ticket?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853753615
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
Sure, you can see failed CI [here](https://github.com/bgeng777/flink-kubernetes-operator/runs/6089734117?check_suite_focus=true#step:10:2083). That branch does not set JM replica to 1 and it should fail due to validation error.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853701580
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/FlinkOperatorITCase.java:
##########
@@ -114,6 +114,7 @@ private static FlinkDeployment buildSessionCluster() {
resource.setCpu(1);
JobManagerSpec jm = new JobManagerSpec();
jm.setResource(resource);
+ jm.setReplicas(1);
Review Comment:
Without this setting, `replicas` in `JobManagerSpec` will be zero be default. That's why the IT test failed.
So do you mean we should change the default value of `replicas` in the class? It seems to be a better solution.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853739465
##########
.gitignore:
##########
@@ -35,3 +35,4 @@ buildNumber.properties
.idea
*.iml
+*.DS_Store
Review Comment:
I notice that when running `git add -A` command, some DS_Store files generated by Mac will be added too. I believe we do not want these files and adding it to gitigore helps developers. Do you have other concerns?
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853740994
##########
.gitignore:
##########
@@ -35,3 +35,4 @@ buildNumber.properties
.idea
*.iml
+*.DS_Store
Review Comment:
I am just curious how the `DS_Store` files are generated.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853845992
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
cd ..
- name: Tests in flink-kubernetes-webhook
Review Comment:
Yes, it is a good suggestion, I believe it is fine to use `verify` on webhook as well. I have updated.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#issuecomment-1103363512
cc @wangyang0918
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853696615
##########
flink-kubernetes-operator/src/test/resources/log4j2-test.properties:
##########
@@ -34,7 +34,7 @@
# limitations under the License.
################################################################################
-rootLogger.level = OFF
+rootLogger.level = INFO
Review Comment:
Using `INFO` log level makes debugging failed tests easier. Hope it does not have other side effects.
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
I am afraid the `org.apache.flink.kubernetes.operator.FlinkOperatorITCase` is not triggered by using `mvn verify`. Could you please confirm this?
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/FlinkOperatorITCase.java:
##########
@@ -114,6 +114,7 @@ private static FlinkDeployment buildSessionCluster() {
resource.setCpu(1);
JobManagerSpec jm = new JobManagerSpec();
jm.setResource(resource);
+ jm.setReplicas(1);
Review Comment:
Why we have to set the replicas explicitly?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853753615
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
Sure, you can see failed CI [here](https://github.com/bgeng777/flink-kubernetes-operator/runs/6089734117?check_suite_focus=true#step:10:2083). This branch does not set JM replica to 1 and it should fail due to validation error.
--
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: issues-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 diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853736459
##########
.gitignore:
##########
@@ -35,3 +35,4 @@ buildNumber.properties
.idea
*.iml
+*.DS_Store
Review Comment:
Why do we need this change?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853753615
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
Sure, you can see failed CI [here](https://github.com/bgeng777/flink-kubernetes-operator/runs/6089734117?check_suite_focus=true#step:10:2083)
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853843673
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/FlinkOperatorITCase.java:
##########
@@ -114,6 +114,7 @@ private static FlinkDeployment buildSessionCluster() {
resource.setCpu(1);
JobManagerSpec jm = new JobManagerSpec();
jm.setResource(resource);
+ jm.setReplicas(1);
Review Comment:
I checked the usage of `replicas` and I think it is better to leave it to another ticket. The change is not big but I hope to make this PR more concentrated on CI workflow improvement.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853755214
##########
.gitignore:
##########
@@ -35,3 +35,4 @@ buildNumber.properties
.idea
*.iml
+*.DS_Store
Review Comment:
Typically, it is because I opened some files(e.g. the CI test report) of this github project with `Finder` in my local machine.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853721497
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
You are right. In the root pom, we only declare `integration-test` goal for `maven-failsafe-plugin` while not adding `verify` goal. As a result, `mvn verify` phase will not execute the `maven-failsafe-plugin:verify`, which makes the build will succeed even when the IT case fails. I have fixed this, the execution of ITCase can be found [here](https://pipelines.actions.githubusercontent.com/serviceHosts/a2c2e851-e8cc-4b87-8abf-a33c133702e9/_apis/pipelines/1/runs/1067/signedlogcontent/2?urlExpires=2022-04-20T04%3A45%3A40.8735995Z&urlSigningMethod=HMACV1&urlSignature=RcXN8qsKPyeGLWBCOCUK9PAi6htDzAb2NQG47FUxpmY%3D).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853721497
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
You are right. In the root pom, we only declare `integration-test` goal for `maven-failsafe-plugin` while not adding `verify` goal. As a result, `mvn verify` phase will not execute the `maven-failsafe-plugin:verify`, which makes the build will succeed even when the IT case fails or is not executed. I have fixed this, the execution of ITCase can be found [here](https://github.com/apache/flink-kubernetes-operator/runs/6089149415?check_suite_focus=true#step:10:2074).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] bgeng777 commented on a diff in pull request #173: [FLINK-27310] Fix FlinkOperatorITCase
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on code in PR #173:
URL: https://github.com/apache/flink-kubernetes-operator/pull/173#discussion_r853721497
##########
.github/workflows/ci.yml:
##########
@@ -73,7 +73,7 @@ jobs:
- name: Tests in flink-kubernetes-operator
run: |
cd flink-kubernetes-operator
- mvn integration-test -Dit.skip=false
+ mvn verify -Dit.skip=false
Review Comment:
You are right. In the root pom, we only declare `integration-test` goal for `maven-failsafe-plugin` while not adding `verify` goal. As a result, `mvn verify` phase will not execute the `maven-failsafe-plugin:verify`, which makes the build will succeed even when the IT case fails. I have fixed this, the execution of ITCase can be found [here](https://github.com/apache/flink-kubernetes-operator/runs/6089149415?check_suite_focus=true#step:10:2074).
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org