Re: [PR] fix(DB update): Gracefully handle querry error during DB update [superset]

Fri, 25 Apr 2025 11:03:21 -0700 


korbit-ai[bot] commented on code in PR #33250:
URL: https://github.com/apache/superset/pull/33250#discussion_r2060673248


##########
superset/commands/database/update.py:
##########
@@ -80,9 +80,19 @@ def run(self) -> Model:
             # existing personal tokens.
             self._handle_oauth2()
 
-        # build new DB
+        # Some DBs require running a query to get the default catalog.
+        # In these cases, if the current connection is broken then
+        # `get_default_catalog` would raise an exception. We need to
+        # gracefully handle that so that the connection can be fixed.
         original_database_name = self._model.database_name
-        original_catalog = self._model.get_default_catalog()
+        force_update: bool = False
+        try:
+            original_catalog = self._model.get_default_catalog()
+        except Exception:
+            original_catalog = None
+            force_update = True

Review Comment:
   ### Overly aggressive catalog update on connection error <sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Setting force_update to True when get_default_catalog fails could lead to 
unnecessary catalog updates for unrelated connection issues.
   
   ###### Why this matters
   If get_default_catalog fails due to a temporary network glitch or unrelated 
error, the code will force update all related assets even when the catalog 
hasn't actually changed, potentially causing unnecessary database operations 
and performance impact.
   
   ###### Suggested change ∙ *Feature Preview*
   Modify the error handling to only force update when necessary, considering 
the specific error type:
   ```python
   try:
       original_catalog = self._model.get_default_catalog()
   except Exception as ex:
       logger.warning(
           'Failed to get default catalog for database %s: %s',
           self._model_id,
           str(ex)
       )
       original_catalog = self._model.catalog
       force_update = self._model.catalog != database.catalog
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e2f684e-b3e8-42ed-a1dc-a998ee883acd/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e2f684e-b3e8-42ed-a1dc-a998ee883acd?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e2f684e-b3e8-42ed-a1dc-a998ee883acd?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e2f684e-b3e8-42ed-a1dc-a998ee883acd?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e2f684e-b3e8-42ed-a1dc-a998ee883acd)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:589ecc6e-7f57-416f-9292-7640e8c3f559 -->
   
   
   [](589ecc6e-7f57-416f-9292-7640e8c3f559)



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


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to