fix: Filter null entities in listPrincipals and listPrincipalRoles to prevent NPE #2258
Conversation
HotSushi
force-pushed
the
npe
branch
2 times, most recently
from
cdaeb0f to
430b04f
Compare
There was a problem hiding this comment.
the fix indeed avoids some NPEs but the fact that similar problems had to be fixed previously (and listCatalogRoles is still suffering from the same problem afaict) makes me believe the problem should be addressed in a more central spot.
from a high level perspective it should not be the responsibility of PolarisAdminService to deal with race-condition issues in the PolarisMetaStoreManager for simple operations like "load all entities of type X".
i've put up #2261 so that we can have a single place for the workaround while future work can look to improve the PolarisMetaStoreManager api in that regard.
if we think the added tests still provide additional value my suggestion would be to rebase your PR after the other PR has been merged, wdyt?
|
Thanks @XN137, agree if there's a better place for the fix to be checked in. Thank you for jumping in. After that fix, I do not think tests would serve additional benefit. Do use them verbatim if you like them in your PR. |
|
Fixing the root cause sounds like the better path forwards. Provided a possible option in #2261 (review) |
`PolarisAdminService` has multiple spots where it is working around the sub-optimal `PolarisMetaStoreManager` APIs. This results in multiple fixes like #1949 and #2258 While eventually the underlying APIs should be improved, for now we can make a single central workaround and clean up some redundant code. Also we can improve the return types as callers are not interested in details of the entity layer.
|
since the other PR has been merged i'd suggest to close this PR now |
github-project-automation
bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
3 participants
Summary
Similar to listCatalogs fix, these methods are non-atomic and can cause NPE when entities are deleted between listEntities and loadEntity calls. Added .filter(Objects::nonNull) to safely handle race conditions.
Similar to: #1949
Tagging reviewers from previous pr:
@eric-maynard
@dimas-b
@andrew4699
Tests
Added tests to verify null filtering behavior when entities are deleted after being listed but before being loaded.
✅
./gradlew buildsucceeds✅ Added unit tests