Hi all, +1 with JB and Ayush.
I do not see a use case for a role cleanup task handler. ENTITY_CLEANUP_SCHEDULER is for table/view storage purge. Roles have no storage assets, and the only registered handler rejects a non-TABLE_LIKE payload. Grant cleanup also does not depend on that flag. On the JDBC path dropEntity already removes grant records in both directions. On NoSQL, stale grant ACLs are cleaned by catalog maintenance. So passing cleanup=false for principal/catalog role deletes looks right to me, same as deletePrincipal. That matches [5686]. +1 as well on JB's follow-up idea: only schedule ENTITY_CLEANUP_SCHEDULER for supported entity types (TABLE_LIKE) so we do not recreate this elsewhere. Thanks Vignesh [5686] https://github.com/apache/polaris/pull/5686 On Wed, 7 Oct 2026 at 19:42, Dmitri Bourlatchkov <[email protected]> wrote: > Hi JB, Ayush, > > Thanks for the analysis! I agree with your points. > > +1 to removing cleanup=false on roles (I already approved that in GH). > > Cheers, > Dmitri. > > On Wed, Oct 7, 2026 at 1:46 AM Jean-Baptiste Onofré <[email protected]> > wrote: > > > Hi Dmitri, > > > > Thanks for this thread. > > > > I took a look on the PR, and I agree with Ayush's analysis: > > 1. The cleanup parameter on dropEntityIfExists and the > > ENTITY_CLEANUP_SCHEDULER task type were specifically intended for > > file/storage purge (Iceberg tables and views). Roles do not have external > > storage assets, so scheduling an async task makes no sense here. > > 2. Dangling grant records are already covered without needing an async > task > > handler. It's sync in JDBC backends (where dropEntity removes grants in > > both directions and updates grant versions) and deferred via catalog > > maintenance in NoSQL. > > 3. deletePrincipal, deleteCatalog, deleteNamespace already pass > > cleanup=false. Setting cleanup=false for deletePrincipalRole and > > deleteCatalogRole aligns them with the rest. It avoids persisting > unhandled > > TASK records that accumulate in the metastore. > > > > There is no need to implement a new role cleanup task handler. I'm +1 to > > pass cleanup=false and update the misleading comments. > > > > As a small follow-up, we might also want to guard dropEntityIfExists in > > TransactionalMetaStoreManagerImpl and AtomicOperationMetaStoreManager so > > that ENTITY_CLEANUP_SCHEDULER tasks are only scheduled for supported > entity > > types (TABLE_LIKE), preventing similar issues elsewhere. > > > > Regards > > JB > > > > On Wed, Oct 7, 2026 at 1:33 AM Dmitri Bourlatchkov <[email protected]> > > wrote: > > > > > Hi All, > > > > > > As discovered in PR [5686] role deletion used to persist task records > > > apparently aim to "cleanup grants" (see PR diff for details). However, > > such > > > tasks cannot be handled by Polaris ATM. > > > > > > Is there a use case for these cleanup tasks? > > > > > > Were they supposed to delete dangling grant records pointing to deleted > > > roles? > > > > > > Is there interest in the community to implement that cleanup behaviour? > > > > > > [5686] https://github.com/apache/polaris/pull/5686 > > > > > > Thanks, > > > Dmitri. > > > > > >
