[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #439: [FLINK-29959] Use optimistic locking when updating the status

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org