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/15 14:38:59 UTC
[GitHub] [flink-kubernetes-operator] SteNicholas opened a new pull request #65: [FLINK-26649] Add startTime in JobStatus
SteNicholas opened a new pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65
The current `JobStatus.updateTime` actually means the start time, not the last update time. We could introduce a new field `startTime` and make `updateTime` reflect its original intention.
**The brief change log**
- Adds the field `startTime` in `JobStatus` to represent the start time.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
tweise commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827219806
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
Have you confirmed that this does not cause a busy reconcile loop? I actually think we should just rename the existing field.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827693565
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
The description in the java doc[1] is not consistent with the documentation.
[1]. https://github.com/java-operator-sdk/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ControllerConfiguration.java#L27
--
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 merged pull request #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 merged pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827791618
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
FYI: it is indeed a typo in the documentation. They have fixed this.
I have verified the status changes will not cause busy reconciliation.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827690962
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
@gyfora So do you mean to need to set `generationAwareEventProcessing` to `false` to enable the filtering? I am afraid the default value is `true`.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827687270
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
@wangyang0918 exactly, we only want to trigger new reconcile automatically when the Spec itself is changed by the user. This does not affect reconciliations that we schedule ourselves but avoids immediate reconciliation triggers when the status changes.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827591600
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
The current javaoperatorsdk behavior is a little strange. When `generationAwareEventProcessing` is configured to `false`, not `true`, then the events will be filtered.
@gyfora This also makes me confused what's your purpose of the commit a2412c0f2434d8005be12318ec347cbe60502753. Do we want to filter the events only when `.spec` changed?
[1]. https://javaoperatorsdk.io/docs/features
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#issuecomment-1068120269
@SteNicholas please run `mvn clean install -Pgenerate-docs -DskipTests` to generate the doc update for the cr reference
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827591600
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
The current javaoperatorsdk behavior is a little strange. When `generationAwareEventProcessing` is configured to `false`, not `true`, then the reconciliation will not be triggered if `.spec` is not changed.
[1]. https://javaoperatorsdk.io/docs/features
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827591600
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
The current javaoperatorsdk behavior is a little strange. When `generationAwareEventProcessing` is configured to `false`, not `true`, then the events will be filtered.
[1]. https://javaoperatorsdk.io/docs/features
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#issuecomment-1069911507
Merging this PR.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827689393
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
GenerationAware here means that it is aware that we should only reconcile if the generation (spec) changes. So it might be a bit confusing but it seems to work :)
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827691956
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
we want it to be aware of the generation so we want it to be `true` which is the default. True enables filtering, false triggers reconcile for every spec/status 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: 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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827695275
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
Yea it should say, `To turn off this feature...`
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827697225
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
I have to verify whether it really works.
--
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 #65: [FLINK-26649] Add startTime in JobStatus
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #65:
URL: https://github.com/apache/flink-kubernetes-operator/pull/65#discussion_r827591600
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobObserver.java
##########
@@ -87,8 +87,8 @@ private void updateJobStatus(JobStatus status, List<JobStatusMessage> clusterJob
status.setState(newJob.getJobState().name());
status.setJobName(newJob.getJobName());
status.setJobId(newJob.getJobId().toHexString());
- // track the start time, changing timestamp would cause busy reconciliation
- status.setUpdateTime(String.valueOf(newJob.getStartTime()));
+ status.setStartTime(String.valueOf(newJob.getStartTime()));
+ status.setUpdateTime(String.valueOf(System.currentTimeMillis()));
Review comment:
The current javaoperatorsdk behavior is a little strange. When `generationAwareEventProcessing` is configured to `false`, not `true`, then the events will be filtered.
@gyfora This also makes me confused about what's your purpose of the commit a2412c0f2434d8005be12318ec347cbe60502753. Do we want to filter the events only when `.spec` changed?
[1]. https://javaoperatorsdk.io/docs/features
--
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