[GitHub] [airflow] potiuk edited a comment on pull request #20546: Allow sensors to return XCom values using tuple

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

potiuk edited a comment on pull request #20546:
URL: https://github.com/apache/airflow/pull/20546#issuecomment-1002476070


   I think this is pretty dangerous if you consider that  sensor implementing this might (and likely will)  be run with Airflow pre 2.3. 
   
   It will work unexpectedly:
   
   ```
   >>> not (True, "Xcom")
   False
   >>> not True
   False
   >>> not (False, "Xcom")
   False
   >>> not False
   True
   ```
   
   As a result, such sensor will always succed if run on Airlfow 2.3.
   
   An interesting thing though, that it's not as bad. This will work fine if (and only if) we make sure that tuple is ONLY returned where the result is "True". And conicidentally - the "XCom" value is really ONLY needed when the returned value is True. There is really no point in storing XCom while the sensor is still running (see below).
   
   So in order to keep compatibiliyt, allowed return values shoudl be one of
   
   * False
   * (True, "Xcom")
   
   That would be difficult to enforce though. But....... This leads me to conclusion that we do not need Tuple AT ALL :).
   
   If we assume that there is no need to store "", False, True or any other "un-truthy" value in XCom we could rather easily make backwards compatible implementation ny changing the contract of `poke` method to return "Truthy" value rather than Bool.
   
   Then
   * in case True or False (Bool) is returned - do not store it in XCom (All Airlfow versions) 
   * in case of "Truthy" non-Bool value is returned - store it in XCom (Airflow 2.3+ only)
   * also (if we really think it is needed) we could store "Falsey" (non Bool) value in Xcom but I think that is really not a good use case for sensors - usually the XCom should only be needed by some tasks that is triggered after sensor is complete).
   
   This willl be backwards compatible - both existing Sensors will work in Airflow 2.3 and new sensors will work on Airlfow pre 2.3 (without storing XCom though).
   
   The only potential incompatibility is that truthy (non-Bool) poke output will also be stored as Xcom (but it is mis-use of sensor - it only supposed to have Bool output pre Airflow 2.3). And it has much better interface than the Tuple IMHO. 
   


-- 
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@airflow.apache.org

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