Add Events for PolarisServiceImpl APIs #2482

adnanhemani · 2025-08-31T06:23:59Z

This PR adds the Events instrumentation for all APIs listed in the PolarisServiceImpl.java file, surrounding the default delegated call to the business logic APIs.

adnanhemani · 2025-08-31T06:25:02Z

...ice/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java


  @Override
  public Response createCatalog(
      CreateCatalogRequest request, RealmContext realmContext, SecurityContext securityContext) {


Remove instrumentation from #1844 for this.

Changed in next revision.

adutra · 2025-09-03T07:34:38Z

...ice/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java

  public Response listCatalogRoles(
      String catalogName, RealmContext realmContext, SecurityContext securityContext) {
-    return delegate.listCatalogRoles(catalogName, realmContext, securityContext);
+    polarisEventListener.onBeforeCatalogList(new CatalogsServiceEvents.BeforeCatalogListEvent());


Is this the right event? We are listing catalog roles, not catalogs.

Good catch!

adutra · 2025-09-03T07:36:12Z

...ice/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java

+            catalogName, catalogRoleName, grantRequest, realmContext, securityContext);
+    PolarisServiceImpl.AddGrantToCatalogRoleEntityWrapper entityWrapper =
+        (PolarisServiceImpl.AddGrantToCatalogRoleEntityWrapper) resp.getEntity();
+    if (resp.getStatus() != Response.Status.BAD_REQUEST.getStatusCode()) {


Suggested change

if (resp.getStatus() != Response.Status.BAD_REQUEST.getStatusCode()) {

if (resp.getStatus() == Response.Status.CREATED.getStatusCode()) {

Removed per comment below.

adutra · 2025-09-03T07:45:04Z

runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

    return Response.ok(catalogRoles).build();
  }

+  record AddGrantToCatalogRoleEntityWrapper(


These records have to be public, as they are potentially escaping the package (IOW, a decorator sitting outside this package would be unable to process the response returned by addGrantToCatalogRole).

These records are OK for now, but we need to include these structures in the OpenAPI spec for this service. Could you please add a TODO for that, or better yet, file an issue? WIthout proper inclusion in the OpenAPI spec, these structures become a form of "undocumented" API.

It seems the problematic endpoints are:

createCatalog

createPrincipalRole

createCatalogRole

addGrantToCatalogRole

revokeGrantFromCatalogRole

All of them would need to have their response schema updated in the OpenAPI spec.

In fact, this topic imo deserves a discussion thread in the ML.

This makes sense, I will start a new ML discussion regarding this. In the meantime, let me add TODOs for this and remove it for now from this PR to unblock the rest of the events.

adutra · 2025-09-03T07:48:31Z

...ice/src/main/java/org/apache/polaris/service/admin/PolarisCatalogsEventServiceDelegator.java

+            entityWrapper.polarisPrivilege(),
+            entityWrapper.grantResource(),
+            entityWrapper.cascade()));
+    return resp;


For now, let's not include the entity in the response, since that would be a violation of the spec.

Agreed, removing as per above comment.

adutra · 2025-09-03T07:53:34Z

runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

        new PrincipalRoleEntity(adminService.createPrincipalRole(entity)).asPrincipalRole();
    LOGGER.info("Created new principalRole {}", newPrincipalRole);
-    return Response.status(Response.Status.CREATED).build();
+    return Response.status(Response.Status.CREATED).entity(newPrincipalRole).build();


nit: we could standardize the usage of toResponse for all endpoints.

Not opposed to this, but let's do this in a different PR :)

adutra · 2025-09-03T07:56:21Z

runtime/service/src/main/java/org/apache/polaris/service/events/CatalogsServiceEvents.java

+import org.apache.polaris.core.entity.PolarisPrivilege;
+
+public class CatalogsServiceEvents {
+  public record BeforeCatalogCreatedEvent(String catalogName) implements PolarisEvent {}


nit: still some naming inconsistencies: most events use a verb in the infinitive form (BeforeCatalogListEvent) but some use the past participle (BeforeCatalogCreatedEvent).

adutra · 2025-09-03T07:59:25Z

runtime/service/src/main/java/org/apache/polaris/service/events/CatalogsServiceEvents.java

+   * @param privilege the privilege granted
+   * @param grantResource the grant resource
+   */
+  public record AfterAddGrantToCatalogRoleEvent(


Ideally, if we eventually modify the OpenAPI spec, I think each "after" event should take a response object that mirrors the "before" event and its request object. E.g.:

record BeforeAddGrantToCatalogRoleEvent(String, String, AddGrantRequest) record AfterAddGrantToCatalogRoleEvent(String, String, AddGrantResponse)

adnanhemani · 2025-09-05T20:55:25Z

runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

        {
-          PolarisPrivilege privilege =
-              PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
+          privilege = PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());


Not a requirement, but is a helpful refactor that will help in the future if/when we change the public API.

flyrain

Thanks @adnanhemani for the PR. LGTM overall. Do we have tests for added events(catalog, principal, principal role)?

flyrain · 2025-09-09T00:57:21Z

runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

-    return Response.status(successStatus).build();
+    Response.ResponseBuilder responseBuilder = Response.status(successStatus);
+    return responseBuilder.build();


This change seems unnecessary.

Good catch, this is a hangover from some other code I removed. Reverted.

This doesn't look reverted yet 🤔

Interesting, I don't know what happened and why it didn't pick up. Thanks for rechecking!

Made the changes now (and have verified)!

adnanhemani · 2025-09-17T20:58:09Z

@flyrain - I haven't added the tests yet, but will add it in a follow up PR. I didn't want to saddle this PR with the extra LOCs for something that's quite straightforward and requires much closer attention to the details of the events themselves.

…gs_principals_principal_roles

flyrain

+1 Thanks @adnanhemani !

adnanhemani added 2 commits August 30, 2025 23:20

Add Events for PolarisServiceImpl APIs

85defa9

spotlessapply

516c492

github-project-automation bot added this to Basic Kanban Board Aug 31, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 31, 2025

adnanhemani commented Aug 31, 2025

View reviewed changes

fix bug

a3c80e0

adnanhemani mentioned this pull request Aug 31, 2025

Add Events for PolarisServiceImpl APIs #1904

Closed

fix bug

668dd84

adnanhemani marked this pull request as ready for review August 31, 2025 08:14

adnanhemani added 2 commits September 2, 2025 22:51

added resetCredentials support

e959201

spotlessapply

ba7e027

adutra reviewed Sep 3, 2025

View reviewed changes

This was referenced Sep 3, 2025

Add Events for Policy Service APIs #2479

Merged

Add Events for Generic Table APIs #2481

Merged

adnanhemani added 3 commits September 5, 2025 13:31

merge from main

495bca4

merge from main again

2d200c3

changes based on comments from @adutra

99a8bf1

adnanhemani commented Sep 5, 2025

View reviewed changes

flyrain reviewed Sep 9, 2025

View reviewed changes

merge from main

6826c98

adnanhemani added 3 commits September 17, 2025 15:19

merge from main

e3d51ed

spotlessapply

1539ef8

revert code change

2df5607

adutra previously approved these changes Sep 19, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 19, 2025

Merge branch 'main' into ahemani/event_instrumentation_polaris_catalo…

06e6d84

…gs_principals_principal_roles

adnanhemani dismissed adutra’s stale review via 06e6d84 September 19, 2025 21:07

flyrain approved these changes Sep 19, 2025

View reviewed changes

flyrain merged commit 553cb06 into apache:main Sep 19, 2025
14 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Sep 19, 2025

	if (resp.getStatus() != Response.Status.BAD_REQUEST.getStatusCode()) {
	if (resp.getStatus() == Response.Status.CREATED.getStatusCode()) {

Add Events for PolarisServiceImpl APIs #2482

Add Events for PolarisServiceImpl APIs #2482

Uh oh!

Conversation

adnanhemani commented Aug 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani commented Sep 17, 2025

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adnanhemani Sep 19, 2025 •

edited

Loading