Add Events for Iceberg REST APIs #2480

adnanhemani · 2025-08-31T06:16:51Z

This PR adds the Events instrumentation for the Iceberg REST Service APIs, surrounding the default delegated call to the business logic APIs.

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

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

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

adutra · 2025-09-01T15:29:07Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

@@ -777,8 +777,8 @@ public Response sendNotification(
        securityContext,
        prefix,
        catalog -> {
-          catalog.sendNotification(tableIdentifier, notificationRequest);
-          return Response.status(Response.Status.NO_CONTENT).build();
+          boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);


Unrelated change?

No, at the moment we do not get whether the notification was sent successfully or not. So I've introduced this change so that we can surface that information. (There are code paths possible where the notificationSent can be false but the call still returns without error).

But to be fair, I'm not sure what notificationSent really helps with. So please let me know what you think.

In this case this requires a modification of the OpenAPI spec. See my related comment:

https://github.com/apache/polaris/pull/2482/files#r2318079257

Also: status(Response.Status.NO_CONTENT).entity(notificationSent) is contradictory: the response for a 204 status should not contain any body:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/204

Needing an OpenAPI spec change and ML thread regarding this is a fair ask. Let me revert this out for now - maybe we can proceed without the result of the sendNotification call first and then in parallel, I can start the ML thread for this.

Once we reach a consensus as a community, then we can go back and resolve the TODO.

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

...rg/apache/polaris/service/catalog/iceberg/IcebergRestConfigurationEventServiceDelegator.java

adutra · 2025-09-03T08:27:10Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

@@ -777,8 +777,8 @@ public Response sendNotification(
        securityContext,
        prefix,
        catalog -> {
-          catalog.sendNotification(tableIdentifier, notificationRequest);
-          return Response.status(Response.Status.NO_CONTENT).build();
+          boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);


In this case this requires a modification of the OpenAPI spec. See my related comment:

https://github.com/apache/polaris/pull/2482/files#r2318079257

adutra · 2025-09-03T08:29:33Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

+  }
+
+  private String getCatalogFromPrefix(String prefix, RealmContext realmContext) {
+    return prefixParser.prefixToCatalogName(realmContext, prefix);


nit: inline.

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

adutra · 2025-09-04T11:08:48Z

...c/main/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListener.java

+import org.apache.polaris.service.events.AfterAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeLimitRequestRateEvent;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;

 public abstract class PolarisPersistenceEventListener extends PolarisEventListener {

  // TODO: Ensure all events (except RateLimiter ones) call `addToBuffer`


Is the plan to tackle this later?

BTW the comment is outdated:

Suggested change

// TODO: Ensure all events (except RateLimiter ones) call `addToBuffer`

// TODO: Ensure all events (except RateLimiter ones) call `processEvent`

Yes, the plan is to tackle in a separate PR - I would like to keep the focus of this PR on the events itself and the transformations will go in a further PR.

singhpk234 · 2025-09-05T21:49:33Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

+  /**
+   * Table Committed Events are already instrumented at a more granular level than the API itself.
+   */


how and why , are it we tracking it per table ?? this seems like it doesn't follow the pattern
-- BeforEvent
-- do Something
-- After Event

The TableCommittedEvent was previously merged and there are no changes to it as part of this PR. The idea is that all table commits that happened as part of the same Polaris API call (through the commitTransaction API) will already be tied together through the same request ID and so a new Event for this API is not explicitly required.

what is the on before and on after for this, would looks though ?

On second thought, you are correct - the Table Committed events are triggered even for non-transactional events. Let me add this.

singhpk234 · 2025-09-05T21:57:15Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

-    return delegate.createTable(
-        prefix, namespace, createTableRequest, accessDelegationMode, realmContext, securityContext);
+    String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+    if (!createTableRequest.stageCreate()) {


why don't we emit anything for StageCreate ?

My understanding is that when a table is stageCreated, then no actual table is made - and therefore there is no new (meta)data created. In that case, I don't think it makes sense to emit a AfterCreateTableEvent if a table was not truly created.

But we are not emitting a onBeforeCreateTable too ? never the less what happens when i stage create and then do /commit ?

I did not realize this - have removed the flag around BeforeCreateTableEvent. Consumers of this event should see this is for a StageCreate table and surface that information accordingly.

singhpk234 · 2025-09-05T22:29:14Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

-    return delegate.listViews(
-        prefix, namespace, pageToken, pageSize, realmContext, securityContext);
+    String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+    polarisEventListener.onBeforeListViews(new BeforeListViewsEvent(catalogName, namespace));


are the nested namespace encoded by '%1F' in the final even ?

This is a good callout - we should switch this to use Namespace objects instead. Doing in the next revision.

singhpk234 · 2025-09-08T21:25:52Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

-    return delegate.createTable(
-        prefix, namespace, createTableRequest, accessDelegationMode, realmContext, securityContext);
+    String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+    if (!createTableRequest.stageCreate()) {


But we are not emitting a onBeforeCreateTable too ? never the less what happens when i stage create and then do /commit ?

singhpk234 · 2025-09-08T21:33:56Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

+  /**
+   * Table Committed Events are already instrumented at a more granular level than the API itself.
+   */


what is the on before and on after for this, would looks though ?

singhpk234 · 2025-09-08T21:35:54Z

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

+    polarisEventListener.onBeforeRenameView(
+        new BeforeRenameViewEvent(catalogName, renameTableRequest));


no namespace here ? wouldn't it be nice to know namespace before after ?

renameTableRequest has the before and after table identifiers, where an event listener can access that the namespace information.

singhpk234

The code changes LGTM from IRC POV ! Thanks @adnanhemani

Orthogonal to the PR - I am a bit concerned with seeing the read events being instrumented too, as my understanding was we wanted write API's only (mirroring event's API), this increases load on persistence and the application in general since we use the same JDBC connection pool and we do batching in memory and reads can shoot these up.
My recommendation as part of follow-ups would be :

Mark this feature experimental, when ever the E2E machinery is ready
run benchmarks with polaris benchmarking tool (whats could be the potential degradation) to identify bottelnecks.

adnanhemani · 2025-09-17T20:23:32Z

Thanks @singhpk234 - makes sense. When we finish the E2E mechanism, we will surely either run the benchmark or mark as experimental.

adnanhemani added 2 commits August 30, 2025 23:08

Add Events for Iceberg REST APIs

6779c52

spotlessapply

93d22ff

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

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java Show resolved Hide resolved

adnanhemani marked this pull request as ready for review August 31, 2025 06:42

adutra reviewed Sep 1, 2025

View reviewed changes

adnanhemani mentioned this pull request Sep 3, 2025

AWS CloudWatch Event Sink Implementation #1965

Merged

adnanhemani added 2 commits September 2, 2025 22:30

spotlessapply

83f35ee

Revision based on review from @adutra

5b19f6f

adutra reviewed Sep 3, 2025

View reviewed changes

merge from main, address comments from @adutra

7dc06dd

adutra previously approved these changes Sep 4, 2025

View reviewed changes

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

merge from main

557621f

adnanhemani dismissed adutra’s stale review via 557621f September 5, 2025 20:57

spotlessapply

ce9a705

singhpk234 reviewed Sep 5, 2025

View reviewed changes

Merge from main, review from @singhpk234

d327da7

singhpk234 reviewed Sep 8, 2025

View reviewed changes

adnanhemani added 2 commits September 8, 2025 15:19

fix compile error

1f9249b

review comments from @singhpk234

f3ef8df

singhpk234 previously approved these changes Sep 9, 2025

View reviewed changes

Merge branch 'main' into ahemani/event_instrumentation_iceberg_rest

78a1aa2

adnanhemani dismissed singhpk234’s stale review via 78a1aa2 September 17, 2025 20:22

merge from main

20295a1

singhpk234 approved these changes Sep 17, 2025

View reviewed changes

singhpk234 merged commit d03c717 into apache:main Sep 17, 2025
14 checks passed

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

dimas-b mentioned this pull request Sep 18, 2025

Unify create/loadTable call paths #2589

Open

	// TODO: Ensure all events (except RateLimiter ones) call `addToBuffer`
	// TODO: Ensure all events (except RateLimiter ones) call `processEvent`

		polarisEventListener.onBeforeRenameView(
		new BeforeRenameViewEvent(catalogName, renameTableRequest));

Add Events for Iceberg REST APIs #2480

Add Events for Iceberg REST APIs #2480

Uh oh!

Conversation

adnanhemani commented Aug 31, 2025

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

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!

singhpk234 left a comment • 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!

Uh oh!

Uh oh!

singhpk234 left a comment •

edited

Loading