morhidi commented on code in PR #439:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/439#discussion_r1023386496
##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
}
public void resetTrigger() {
- this.triggerId = "";
+ this.triggerId = null;
this.triggerTimestamp = 0L;
Review Comment:
Can't we use `Long` and `null`s as default for the timestamps as well
(lastPeriodicSavepointTimestamp,triggerTimestamp) or it would break backward
compatibility?
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/StatusRecorder.java:
##########
@@ -97,20 +96,79 @@ public void patchAndCacheStatus(CR resource) {
Exception err = null;
for (int i = 0; i < 3; i++) {
- // In any case we retry the status update 3 times to avoid some
intermittent
- // connectivity errors if any
+ // We retry the status update 3 times to avoid some intermittent
connectivity errors
try {
- client.resource(resource).patchStatus();
- statusUpdateListener.accept(resource, prevStatus);
- metricManager.onUpdate(resource);
- return;
- } catch (Exception e) {
+ replaceStatus(resource, prevStatus);
+ err = null;
+ } catch (KubernetesClientException e) {
LOG.error("Error while patching status, retrying {}/3...", (i
+ 1), e);
Thread.sleep(1000);
err = e;
}
}
- throw err;
+
Review Comment:
I trust you with this @gyfora. Do you think we should add some multithreaded
unit tests for this or you verified it locally.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
