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/02/24 16:54:48 UTC
[GitHub] [flink-kubernetes-operator] bgeng777 opened a new pull request #23: [FLINK-26178] Use the same enum for expected and observed jobstate
bgeng777 opened a new pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23
- Rename the `state` field in `JobStatus` to `jobState` and set its type to `JobState` enum.
--
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 #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814104851
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/JobStatus.java
##########
@@ -30,7 +32,7 @@
public class JobStatus {
private String jobName;
private String jobId;
- private String state;
Review comment:
So to summarize, my suggestion is to not change the name of the field in this PR, but instead let's open a ticket where we gather some improvements to the spec/status instead.
--
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] bgeng777 commented on a change in pull request #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814452578
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/JobStatus.java
##########
@@ -30,7 +32,7 @@
public class JobStatus {
private String jobName;
private String jobId;
- private String state;
Review comment:
Sure. It makes sense to me to leave the name change to another ticket. The renaming has been rolled back.
The reason of renaming here is to follow the naming convention of `org.apache.flink.runtime.client.JobStatusMessage` of flink. We could discuss if such change is necessary later.
--
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] bgeng777 commented on a change in pull request #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814535917
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobStatusObserver.java
##########
@@ -95,7 +95,7 @@ private JobStatus mergeJobStatus(
if (newStatus == null) {
newStatus = createJobStatus(newJob);
} else {
- newStatus.setState(newJob.getJobState().name());
+ newStatus.setState(JobState.valueOf(newJob.getJobState().name()));
Review comment:
Hi @gyfora thanks for sharing this point and I think about it as well and I totally agree that there is some error handle missing here.
I discuss it a little in the [jira](https://issues.apache.org/jira/browse/FLINK-26178). I think using the job state enum should be reasonable as this state itself is an enum. I believe the key is [FLINK-26139](https://issues.apache.org/jira/browse/FLINK-26139) has not been resolved properly which makes the error handle in this part is misleading. I would spend some time on FLINK-26139, hoping to propose a state machine to describe the job state transition later.
This PR seems to be done a little early. We may finish it after we resolve FLINK-26139. What's your advice?
--
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] bgeng777 commented on pull request #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#issuecomment-1050674284
Will start refining this PR once [FLINK-26139](https://issues.apache.org/jira/browse/FLINK-26139) is done.
--
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] bgeng777 closed pull request #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
bgeng777 closed pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23
--
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 #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814101642
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/JobStatus.java
##########
@@ -30,7 +32,7 @@
public class JobStatus {
private String jobName;
private String jobId;
- private String state;
Review comment:
Why are we renaming this and not other fields? I agree that it is not super consistent we
jobName, jobId, state, updateTime, savepointLocation
Wouldnt it be better to have simply name, id, state ...
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/status/JobStatus.java
##########
@@ -30,7 +32,7 @@
public class JobStatus {
private String jobName;
private String jobId;
- private String state;
Review comment:
Maybe this could be a separate JIRA discussion to improve field naming in the spec/status and do multiple changes together to simplify testing (redeploying CRD etc.)
--
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 #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814563646
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobStatusObserver.java
##########
@@ -95,7 +95,7 @@ private JobStatus mergeJobStatus(
if (newStatus == null) {
newStatus = createJobStatus(newJob);
} else {
- newStatus.setState(newJob.getJobState().name());
+ newStatus.setState(JobState.valueOf(newJob.getJobState().name()));
Review comment:
Yea I agree we need to first figure out how exactly we want to handle the state transitions and react to different job states. I am sorry to have wasted your time with this ticket, I should have thought more about it before opening it.
--
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 #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814518383
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobStatusObserver.java
##########
@@ -95,7 +95,7 @@ private JobStatus mergeJobStatus(
if (newStatus == null) {
newStatus = createJobStatus(newJob);
} else {
- newStatus.setState(newJob.getJobState().name());
+ newStatus.setState(JobState.valueOf(newJob.getJobState().name()));
Review comment:
This will give an ugly error if the job is not in a running state. Not sure how to go around this or even if it makes sense to use the operator jobstate enum here after all
--
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] bgeng777 commented on a change in pull request #23: [FLINK-26178] Use the same enum for expected and observed jobstate
Posted by GitBox <gi...@apache.org>.
bgeng777 commented on a change in pull request #23:
URL: https://github.com/apache/flink-kubernetes-operator/pull/23#discussion_r814598597
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobStatusObserver.java
##########
@@ -95,7 +95,7 @@ private JobStatus mergeJobStatus(
if (newStatus == null) {
newStatus = createJobStatus(newJob);
} else {
- newStatus.setState(newJob.getJobState().name());
+ newStatus.setState(JobState.valueOf(newJob.getJobState().name()));
Review comment:
Oh, it doesn't matter. The work is not a waste at all, I may just need to refine it once we achieve consensus on the state transition. Thanks a lot for the above review and I will close this PR and may reopen it when [FLINK-26139](https://issues.apache.org/jira/browse/FLINK-26139) is done.
--
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