uranusjr commented on PR #76826: URL: https://github.com/apache/airflow/pull/76826#issuecomment-6794600044
Hmm, I was focusing on Get since that’s what task mapping needs. However, Delete is dangerous, and I feel removing _all_ the XComs for the task is you happen to forget to pass `map_index` is too big a footgun. (Doing this with a non-mapped task, or passing a valid map index for a mapped one already both behave correctly.) I can think of two alternatives: 1. Stay as-is and document. This is pretty safe. Since a mapped task would not have a -1 ti, the endpoint would silently do nothing if you forget the parameter. The only problem is that, currently, deleting a non-existent XCom returns 200, so this could be difficult to debug. 2. Have the server return an error (maybe 400) instead. This is justifiable since requesting -1 for a mapped task does not make sense in the first place. I think we probably also want to change the default too for consistency, but the change of default value would not be noticeable by users. We could also If we don’t change the default (-1), it’s also worth discussing whether an _explicit_ `map_index=None` deleting all XComs should be allowed. It can be useful. Either way, this is probably worth a separate issue and PR(s). The user-facing change would be minimal, only involving edge cases that don’t currently do anything meaningful in the first place. What do you think? -- 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]
