jscheffl commented on code in PR #41414:
URL: https://github.com/apache/airflow/pull/41414#discussion_r1714438001


##########
airflow/models/variable.py:
##########
@@ -167,6 +166,35 @@ def set(
 
         This operation overwrites an existing variable.
 
+        :param key: Variable Key
+        :param value: Value to set for the Variable
+        :param description: Description of the Variable
+        :param serialize_json: Serialize the value to a JSON string
+        :param session: Session
+        """
+        Variable._set(
+            key=key, value=value, description=description, 
serialize_json=serialize_json, session=session
+        )
+        # invalidate key in cache for faster propagation
+        # we cannot save the value set because it's possible that it's 
shadowed by a custom backend
+        # (see call to check_for_write_conflict above)
+        SecretCache.invalidate_variable(key)

Review Comment:
   What Vincent means is that is no isolation mode is on, the internal API call 
is made directly in python and effectively the 3 write operations each two 
times flush the cache. (before and behind the internal API facade).
   
   I could wrap an `if` around if internal API is not active and save the 
effort of a second flush of cache. That would save a few CPU cycles. I would 
assume a second flush is not too expensive compared to the mental load of an 
additional if statement. I'd leave it to your opinion.... an if around the 
second flush only make it if internal API active or two flushes but simpler 
code?
   
   As of distributed setup I assumed I need to flush cache on client as well as 
API server side? (How is it today if two workers act concurrently it could 
happen that cached variables are out-of-sync... remember the discussion on 
devlist... in this regards we could also drop the second flush on the server 
side going back to the discussion about the risk of inconsistencies because of 
distributed cache? Could also be multiple instances of webserver/internal API 
server each having a different state in cache...)



-- 
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]

Reply via email to