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/14 16:04:16 UTC
[GitHub] [flink-kubernetes-operator] Aitozi opened a new pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Aitozi opened a new pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58
… have changed
This PR is meant to eliminate the duplicate status updating for `FlinkDeployment`. To achieve this, we compare the object with the original copy before reconcile. If status changed, the `UpdateControl` will apply, otherwise, It will produce no update.
--
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] Aitozi commented on a change in pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
Aitozi commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826691330
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/exception/DeploymentFailedException.java
##########
@@ -26,6 +26,7 @@
/** Exception to signal terminal deployment failure. */
public class DeploymentFailedException extends RuntimeException {
public static final String COMPONENT_JOBMANAGER = "JobManagerDeployment";
+ private static final long serialVersionUID = -1070179896083579221L;
Review comment:
All the `Exception` class are `Serializable`, So we should add `serialVersionUIDs` for them. I will keep in mind to set the `serialVersionUIDs` to `1L` when introduce serializable class next 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] Aitozi commented on a change in pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
Aitozi commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826619853
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobManagerDeploymentStatus.java
##########
@@ -45,7 +46,9 @@
ERROR;
public UpdateControl<FlinkDeployment> toUpdateControl(
- FlinkDeployment flinkDeployment, FlinkOperatorConfiguration operatorConfiguration) {
+ FlinkDeployment originalCopy,
Review comment:
Agree, I will improve 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] Aitozi commented on pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
Aitozi commented on pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#issuecomment-1067487304
cc @wangyang0918 @gyfora
--
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 #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826616116
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobManagerDeploymentStatus.java
##########
@@ -45,7 +46,9 @@
ERROR;
public UpdateControl<FlinkDeployment> toUpdateControl(
- FlinkDeployment flinkDeployment, FlinkOperatorConfiguration operatorConfiguration) {
+ FlinkDeployment originalCopy,
Review comment:
Maybe instead of adding `originalCopy` here we could simply change this method to return the reschedule delay only. I think that would fit better in the current flow. Then we can use the `ReconciliationUtils.toUpdateControl` in the controller
What do you think?
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/exception/DeploymentFailedException.java
##########
@@ -26,6 +26,7 @@
/** Exception to signal terminal deployment failure. */
public class DeploymentFailedException extends RuntimeException {
public static final String COMPONENT_JOBMANAGER = "JobManagerDeployment";
+ private static final long serialVersionUID = -1070179896083579221L;
Review comment:
Why did you add the serialVersionUIDs?
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/ReconciliationUtils.java
##########
@@ -74,4 +77,22 @@ public static void updateForReconciliationError(FlinkDeployment flinkApp, String
throw new IllegalStateException(e);
}
}
+
+ public static UpdateControl<FlinkDeployment> toUpdateControl(
+ FlinkDeployment originalCopy, FlinkDeployment current) {
+ UpdateControl<FlinkDeployment> updateControl;
+ if (!Objects.equals(originalCopy.getSpec(), current.getSpec())) {
+ throw new UnsupportedOperationException(
+ "The spec changed during reconcile is not supported.");
Review comment:
I think this is a valid safeguard but I think the error should be something like:
```
Detected spec change after reconcile, this probably indicates a bug.
```
--
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 #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
gyfora commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826656651
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobManagerDeploymentStatus.java
##########
@@ -44,7 +41,7 @@
/** Deployment in terminal error, requires spec change for reconciliation to continue. */
ERROR;
- public UpdateControl<FlinkDeployment> toUpdateControl(
+ public Time rescheduleAfter(
Review comment:
I think we should use the built in java Duration class instead of the Flink 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] wangyang0918 commented on a change in pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826682501
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/exception/DeploymentFailedException.java
##########
@@ -26,6 +26,7 @@
/** Exception to signal terminal deployment failure. */
public class DeploymentFailedException extends RuntimeException {
public static final String COMPONENT_JOBMANAGER = "JobManagerDeployment";
+ private static final long serialVersionUID = -1070179896083579221L;
Review comment:
nit: we could always set the `serialVersionUIDs` to `1L` when introduce a serializable class. BTW, do we really need to serialize `DeploymentFailedException`?
https://flink.apache.org/contributing/code-style-and-quality-java.html
--
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] Aitozi commented on a change in pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
Aitozi commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826619398
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/exception/DeploymentFailedException.java
##########
@@ -26,6 +26,7 @@
/** Exception to signal terminal deployment failure. */
public class DeploymentFailedException extends RuntimeException {
public static final String COMPONENT_JOBMANAGER = "JobManagerDeployment";
+ private static final long serialVersionUID = -1070179896083579221L;
Review comment:
The IDE complains this
--
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] Aitozi commented on a change in pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
Aitozi commented on a change in pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#discussion_r826661895
##########
File path: flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/JobManagerDeploymentStatus.java
##########
@@ -44,7 +41,7 @@
/** Deployment in terminal error, requires spec change for reconciliation to continue. */
ERROR;
- public UpdateControl<FlinkDeployment> toUpdateControl(
+ public Time rescheduleAfter(
Review comment:
Fixed
--
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] Aitozi commented on pull request #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
Aitozi commented on pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#issuecomment-1067658054
Updated according to your comments, PTAL @gyfora
--
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 #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
gyfora commented on pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58#issuecomment-1067662008
Added one last minor comment :)
--
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 #58: [FLINK-26528] Trigger the updateControl only when the FlinkDeployment…
Posted by GitBox <gi...@apache.org>.
gyfora merged pull request #58:
URL: https://github.com/apache/flink-kubernetes-operator/pull/58
--
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