Re: [PR] fix: Check migration status before initializing database-dependent features [superset]

[via GitHub]

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


##########
superset/initialization/__init__.py:
##########
@@ -503,11 +504,40 @@ def init_views(self) -> None:
             icon="fa-lock",
         )
 
+    def _is_database_up_to_date(self) -> bool:
+        """
+        Check if database migrations are up to date.
+        Returns False if there are pending migrations or unable to determine.
+        """
+        try:
+            # Get current revision from database
+            with db.engine.connect() as connection:
+                context = MigrationContext.configure(connection)
+                current_rev = context.get_current_revision()

Review Comment:
   ### Uncached Database Migration Check <sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The database migration check creates a new connection for each verification 
without any caching mechanism, potentially causing unnecessary database 
connections on every app startup.
   
   
   ###### Why this matters
   Multiple apps/workers starting simultaneously could create connection spikes 
to the database. For migration status that rarely changes, this is inefficient.
   
   ###### Suggested change ∙ *Feature Preview*
   Implement caching of the migration status with a reasonable TTL (e.g. 5 
minutes) to avoid repeated database connections:
   ```python
   def _is_database_up_to_date(self) -> bool:
       cache_key = 'db_migration_status'
       if status := cache_manager.get(cache_key):
           return status
           
       try:
           with db.engine.connect() as connection:
               context = MigrationContext.configure(connection)
               current_rev = context.get_current_revision()
               
           alembic_cfg = Config()
           alembic_cfg.set_main_option("script_location", "superset:migrations")
           script = ScriptDirectory.from_config(alembic_cfg)
           head_rev = script.get_current_head()
           
           is_current = current_rev == head_rev
           cache_manager.set(cache_key, is_current, timeout=300)  # Cache for 5 
minutes
           return is_current
       except Exception as e:
           logger.debug("Could not check migration status: %s", e)
           return False
   ```
   
   
   ###### 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/9a5e4c81-ba56-4641-bda9-c67fc604bb4a/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5e4c81-ba56-4641-bda9-c67fc604bb4a?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/9a5e4c81-ba56-4641-bda9-c67fc604bb4a?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/9a5e4c81-ba56-4641-bda9-c67fc604bb4a?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a5e4c81-ba56-4641-bda9-c67fc604bb4a)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:1319256c-6252-4a9a-a82a-9057953a878e -->
   
   
   [](1319256c-6252-4a9a-a82a-9057953a878e)



-- 
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: notifications-unsubscr...@superset.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org