Re: [PR] Retry transient database errors in the metastore secrets backend [airflow]

["ferruzzi (via GitHub)" <gi...@apache.org>]

ferruzzi commented on PR #71307:
URL: https://github.com/apache/airflow/pull/71307#issuecomment-5610564977

   First, I'm sorry this sat over a month without any reviews, we really got swarmed with AI-generated contributions and didn't (still don't, TBH) have a good way to filter them out.  
   
   That said, nice troubleshooting.  A permanent 404 for a secret that exists is a real bug, and the `None` caching makes it worse.  But I think the retry is a bandaid rather than the fix.  Once you exhaust `max_db_retries`, you still get the same 404 and it is still cached for `cache_ttl_seconds`.
   
   The root cause looks like it is that both callers accept a database failure as missing data. `get_connection_from_secrets` already distinguishes one class of error:
   
   ```python
   except AirflowSecretsBackendAccessDenied:
       # Authoritative deny — must NOT fall through to a less-restrictive backend.
       raise
   except Exception:
       ...fall through...
   ```
   
   ...and then, after the loop, `raise AirflowNotFoundException(f"The conn_id {conn_id} isn't defined")`.  `(DBAPIError, StaleDataError)` belongs in that carve-out for exactly the same reason: it isn't an answer about whether the secret exists.
   
   Here's what I'd suggest: keep trying the remaining backends since a metastore blip shouldn't stop Vault from answering, but track that a backend errored.  Then, if nothing is found, surface a database error rather than not-found, and skip the `save_variable(key, None)` in that case.
   
   One more reason to prefer that over the decorator: `retry_db_transaction` calls `session.rollback()`, and `MetastoreBackend.get_variable` takes a `session` argument that callers will be supplying once #71968 lands.  That means it can roll back a transaction it doesn't own, which is the problem #71968 exists to avoid.  Fixing the classification sidesteps the session question entirely.
   
   Happy to be argued out of it if you think the retry is the better solution.  It's an improvement either way, just not one that closes the 404 bug.
   


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