Introducing AfterRefreshTableEventListener and Mixins #2836

vchag · 2025-10-17T15:06:06Z

These changes were created to address concerns raised in Issues #2630

What changes were proposed in this pull request?

Renaming PropertyMapEventListener for clarity:
The class PropertyMapEventListener was misleadingly named, suggesting a generic event listening capability when its only overridden method was onAfterRefreshTable. To ensure the name accurately reflects its single responsibility, we have renamed it to AfterRefreshTableEventListener. This precise naming clearly communicates its purpose: to only process the AfterRefreshTableEvent.
Streamlining Event-to-JSON Serialization:
The previous architecture involved a redundant, two-step conversion process: events were transformed into intermediate Maps by an abstract class, and only then was the resulting map serialized into JSON (as seen in AwsCloudWatchEventListener).
Since Jackson JSON is our chosen serialization format, we've removed this unnecessary intermediate mapping step. We introduced Jackson Mixins to provide native JSON serialization support directly on the event objects. This refactoring simplifies the serialization pipeline from:
Event -> Map -> JSON
to
Event -> JSON

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

CHANGELOG.md

adutra

Thanks @vchag the mixin approach looks interesting. There are however a few items that need fixing before we can merge.

adutra · 2025-10-21T12:12:48Z

...service/src/main/java/org/apache/polaris/service/events/json/mixins/ObjectMapperFactory.java

+public class ObjectMapperFactory {
+  private ObjectMapperFactory() {}
+
+  public static ObjectMapper create() {


You should not create an ObjectMapper like this, but rather use CDI.

If possible, let's use the main ObjectMapper provided by Quarkus. You can customize it by adding your customizations in the org.apache.polaris.service.config.PolarisIcebergObjectMapperCustomizer bean.

If it's not possible to reuse the main ObjectMapper (for example, because of incompatible configuration), then you should produce a new ObjectMapper bean.

I realize that the old version of AwsCloudWatchEventListener was also doing it wrong, so let's seize the opportunity to fix this.

I agree with the CDI based approach. I found option #2 to be better feasible.

Let's seize the opportunity!

adutra · 2025-10-21T12:16:51Z

...service/src/main/java/org/apache/polaris/service/events/json/mixins/ObjectMapperFactory.java

+            .registerModule(new Jdk8Module()) // If you never serialize Optional, you can remove the
+            // .registerModule(new Jdk8Module()) line.
+            .registerModule(new JavaTimeModule())
+            .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)


Can you explain why we need to disable these features? I would prefer to customize the mapper as little as possible.

This was an oversight on my part.... will remove them.

adutra · 2025-10-21T12:20:22Z

...ice/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergThirdPartyMixins.java

+ * Mixins for Iceberg classes we don't control, to keep JSON concise. The @JsonValue marks
+ * toString() as the value to serialize.
+ */
+public class IcebergThirdPartyMixins {


nit: rename to IcebergMixins and make it final.

Will rename.

adutra · 2025-10-21T12:32:28Z

runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.java

+public class IcebergThirdPartyMixins {
+  private IcebergThirdPartyMixins() {}
+
+  public abstract static class NamespaceMixin {


I wonder if this is the best thing to do when serializing namespaces and table identifiers. Granted, it's very convenient, but it would not correctly handle dots in namespace segments.

For example the following namespaces would produce the same string:

Namespace.of("one.two", "three.four", "ns"); Namespace.of("one", "two", "three", "four", "ns");

I think it's wiser to serialize these two classes as objects, not scalars.

Great catch!

How about we aligned the serialization behavior with how the Iceberg REST API represents namespaces and table identifiers?

Namespace.of("one", "two", "three", "four", "ns");
would result in:
{"namespace": ["one", "two", "three", "four", "ns"]}

TableIdentifier.of(Namespace.of("one", "two", "three", "four", "ns"), "table")
would result in:
{"namespace": ["one", "two", "three", "four", "ns"], "name": "table"}

Indeed I think that's preferable. It's slightly verbose for simple cases, but it's the only representation that is compatible with all the corner cases.

adutra · 2025-10-21T12:34:39Z

...ain/java/org/apache/polaris/service/events/json/serde/TableIdentifierToStringSerializer.java

+import java.io.IOException;
+import org.apache.iceberg.catalog.TableIdentifier;
+
+public class TableIdentifierToStringSerializer extends JsonSerializer<TableIdentifier> {


Why do we need this serializer? We already have IcebergThirdPartyMixins.TableIdentifierMixin.

This is redundant. I'll go ahead and remove it.

adutra · 2025-10-21T12:35:24Z

...ache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java

+      String realmId,
+      Collection<String> activatedRoles,
+      String eventType,
+      @JsonUnwrapped PolarisEvent event // flatten


adutra · 2025-10-21T12:44:52Z

...java/org/apache/polaris/service/events/jsonEventListener/AfterRefreshTableEventListener.java

 */
-public abstract class PropertyMapEventListener implements PolarisEventListener {
-  protected abstract void transformAndSendEvent(HashMap<String, Object> properties);
+public abstract class AfterRefreshTableEventListener implements PolarisEventListener {


I'm sorry but just renaming this class doesn't make it any more usable.

In its current form, this class does not qualify as a general-purpose component worthy of being distributed with Polaris OSS.

In my opinion this class must be removed.

BTW, AwsCloudWatchEventListener is also extremely problematic, as it only handles after-table-refresh events, and nothing else. In theory, AwsCloudWatchEventListener should either equally handle all 150+ event types, or make the event types to handle configurable.

adutra · 2025-10-21T12:45:47Z

...java/org/apache/polaris/service/events/jsonEventListener/AfterRefreshTableEventListener.java

 * under the License.
 */

 package org.apache.polaris.service.events.jsonEventListener;


We also need to modify this package name: jsonEventListener does not comply with java naming conventions:

https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html

How about we re-name it listener?

adutra · 2025-10-21T12:48:44Z

...src/main/java/org/apache/polaris/service/events/json/mixins/AfterRefreshTableEventMixin.java

+import org.apache.polaris.service.events.json.serde.TableIdentifierToStringSerializer;
+
+@JsonTypeName("AfterRefreshTableEvent")
+public abstract class AfterRefreshTableEventMixin {


I don't think you need this mixin, we already have specified @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) in PolarisEventBaseMixin. That should be enough.

Actually, Yes. We don't need this mixin.

adutra · 2025-10-21T12:56:29Z

...ache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java

-    properties.put(
-        "activated_roles", ((PolarisPrincipal) securityContext.getUserPrincipal()).getRoles());
-    // TODO: Add request ID when it is available
+  protected void transformAndSendEvent(IcebergRestCatalogEvents.AfterRefreshTableEvent event) {


As said above, this class must handle all event types, or determine the event types to handle via configuration.

For example, we could introduce the following configuration option:

polaris.event-listener.aws-cloudwatch.event-types=\ org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent,\ org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterCommitTableEvent

I would be in favor of doing this change in this PR since it this class has the same shortcomings as PropertyMapEventListener.

Oh right. Lets introduce a new config for aws-cloudwatch.event-types.

adutra · 2025-10-27T11:31:56Z

runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.java

+public class IcebergThirdPartyMixins {
+  private IcebergThirdPartyMixins() {}
+
+  public abstract static class NamespaceMixin {


Indeed I think that's preferable. It's slightly verbose for simple cases, but it's the only representation that is compatible with all the corner cases.

adutra · 2025-10-27T11:32:45Z

runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.java

-    @JsonValue
-    public abstract String toString(); // serializes "namespace" as "db.sales"
+    @JsonIgnore
+    public abstract String toString();


Do we still need to declare this method?

adutra · 2025-10-27T11:32:54Z

runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.java

-    @JsonValue
-    public abstract String toString(); // serializes "table_identifier" as "db.sales.orders"
+    @JsonIgnore
+    public abstract String toString();


adutra · 2025-10-27T11:34:03Z

...in/java/org/apache/polaris/service/events/jsonEventListener/AllEventsForwardingListener.java

+
+/**
+ * Base class for event listeners that with to generically forward all {@link PolarisEvent
+ * PolarisEvents} to an external sinks


Suggested change

* PolarisEvents} to an external sinks

* PolarisEvents} to an external sink.

adutra · 2025-10-27T12:32:21Z

...rc/main/java/org/apache/polaris/service/config/PolarisAWSCloudWatchObjectMapperProducer.java

+import org.slf4j.LoggerFactory;
+
+/** This mapper is isolated and used exclusively for CloudWatch event serializations */
+public class PolarisAWSCloudWatchObjectMapperProducer {


Why do we need a separate mapper? This one seems fairly identical to the default one, except of course for the mixins. But you could add the mixins to the default mapper instead.

adutra · 2025-10-27T12:34:46Z

...rvice/quarkus/events/jsonEventListener/aws/cloudwatch/QuarkusAwsCloudWatchConfiguration.java

+  @WithName("event-types")
+  @WithDefault("org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent")
+  @Override
+  Set<String> eventTypes();


I think this could be Set<Class<? extends PolarisEvent>>.

adutra · 2025-10-27T12:37:23Z

...rvice/quarkus/events/jsonEventListener/aws/cloudwatch/QuarkusAwsCloudWatchConfiguration.java

+   * @return a set of event types
+   */
+  @WithName("event-types")
+  @WithDefault("org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent")


That's not a good default. I'd say the default should be an empty set, and we should treat an empty set as "process all events".

WDYT?

Also, please add the corresponding property to application.properties, that helps users:

polaris/runtime/defaults/src/main/resources/application.properties

Lines 142 to 147 in 0acbb64

# AWS CloudWatch event listener settings

# polaris.event-listener.type=aws-cloudwatch

# polaris.event-listener.aws-cloudwatch.log-group=polaris-cloudwatch-default-group

# polaris.event-listener.aws-cloudwatch.log-stream=polaris-cloudwatch-default-stream

# polaris.event-listener.aws-cloudwatch.region=us-east-1

# polaris.event-listener.aws-cloudwatch.synchronous-mode=false

adutra · 2025-10-27T12:40:53Z

...rc/main/java/org/apache/polaris/service/config/PolarisAWSCloudWatchObjectMapperProducer.java

+
+  @Inject
+  public PolarisAWSCloudWatchObjectMapperProducer(
+      @ConfigProperty(name = "polaris.aws.cloudwatch.max-body-size", defaultValue = "16M")


Where is this config property coming from? It doesn't start with the prefix used for other CloudWatch properties, which is polaris.event-listener.aws-cloudwatch..

adutra · 2025-10-27T12:42:04Z

site/content/in-dev/unreleased/configuration.md

+| `polaris.tasks.max-concurrent-tasks`                                                   | `100`                            | Define the max number of concurrent tasks.                                                                                                                                                                                                                                                                                                                    |
+| `polaris.tasks.max-queued-tasks`                                                       | `1000`                           | Define the max number of tasks in queue.                                                                                                                                                                                                                                                                                                                      |
+| `polaris.config.rollback.compaction.on-conflicts.enabled`                              | `false`                          | When set to true Polaris will apply the deconfliction by rollbacking those REPLACE operations snapshots which have the property of `polaris.internal.rollback.compaction.on-conflict` in their snapshot summary set to `rollback`, to resolve conflicts at the server end.                                                                                    |
+| `polaris.event-listener.type`                                                          | `no-op`                          | Define the Polaris event listener type. Supported values are `no-op`, `aws-cloudwatch`.                                                                                                                                                                                                                                                                       |


BTW persistence-in-memory-buffer is also supported.

adutra · 2025-10-27T12:43:15Z

site/content/in-dev/unreleased/configuration.md

 | `polaris.event-listener.aws-cloudwatch.log-stream`                                     | `polaris-cloudwatch-default-stream`| Define the AWS CloudWatch log stream name for the event listener. Ensure that Polaris' IAM credentials have the following actions: "PutLogEvents", "DescribeLogStreams", and "DescribeLogGroups" on the specified log stream/group. If the specified log stream/group does not exist, then "CreateLogStream" and "CreateLogGroup" will also be required.      |
-| `polaris.event-listener.aws-cloudwatch.region`                                         | `us-east-1`                        | Define the AWS region for the CloudWatch event listener.                                                                                                                                                                                                                                                                                                      |
-| `polaris.event-listener.aws-cloudwatch.synchronous-mode`                               | `false`                            | Define whether log events are sent to CloudWatch synchronously. When set to true, events are sent synchronously which may impact performance but ensures immediate delivery. When false (default), events are sent asynchronously for better performance.                                                                                                     |
-


Nit: Why remove this line?

vchag · 2025-10-28T03:21:05Z

Closing in favor of a new PR from a feature branch.
My fork’s main fell behind upstream and a fork sync/force-push unintentionally rewrote the PR head, which auto-closed this PR.
I'll recreate the changes on a proper feature branch, rebased on the latest upstream/main, and addressed prior feedback.

Thanks for the reviews so far.....carrying over all context to the new PR.

adutra · 2025-10-30T13:49:58Z

@vchag what's the status here? Are you still able to work in this issue?

vchag · 2025-11-03T03:08:26Z

@adutra Yes, I'm on it. Will release it within the next 4 - 8 hrs.

vchag · 2025-11-03T06:29:51Z

Here's the new PR #2962.

github-project-automation bot added this to Basic Kanban Board Oct 17, 2025

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

flyrain requested a review from adutra October 17, 2025 17:12

adutra reviewed Oct 21, 2025

View reviewed changes

adutra mentioned this pull request Oct 24, 2025

Prep: Site for 1.2 release #2877

Merged

adutra reviewed Oct 27, 2025

View reviewed changes

vchag closed this Oct 28, 2025

vchag force-pushed the main branch from 1ccdefd to f414b85 Compare October 28, 2025 01:53

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Oct 28, 2025

	* PolarisEvents} to an external sinks
	* PolarisEvents} to an external sink.

	# AWS CloudWatch event listener settings
	# polaris.event-listener.type=aws-cloudwatch
	# polaris.event-listener.aws-cloudwatch.log-group=polaris-cloudwatch-default-group
	# polaris.event-listener.aws-cloudwatch.log-stream=polaris-cloudwatch-default-stream
	# polaris.event-listener.aws-cloudwatch.region=us-east-1
	# polaris.event-listener.aws-cloudwatch.synchronous-mode=false

Introducing AfterRefreshTableEventListener and Mixins #2836

Introducing AfterRefreshTableEventListener and Mixins #2836

Uh oh!

Conversation

vchag commented Oct 17, 2025

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

CHANGELOG.md

Uh oh!

adutra 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!

vchag Oct 26, 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!

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!

adutra Oct 21, 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!

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!

vchag commented Oct 28, 2025

Uh oh!

adutra commented Oct 30, 2025

Uh oh!

vchag Oct 26, 2025 •

edited

Loading

adutra Oct 21, 2025 •

edited

Loading

vchag commented Nov 3, 2025 •

edited

Loading