PropertyMapEventListener is incomplete / underspecified

[adutra]

Describe the bug

Currently, PropertyMapEventListener implements only one method from PolarisEventListener – all others are ignored. It is unclear if this is on purpose.

Furthermore:

• The API is not ideal: transformAndSendEvent takes a HashMap parameter, and it is unclear whether the requirement for a HashMap (instead of Map) is legit.
• The only implemented method – onAfterRefreshTable does not serialize all the event fields. Again, it is unclear if this is by design or an oversight.

And finally, the whole point of having an abstract class that transforms events into Maps, only to have the Maps transformed into Json (cf. AwsCloudWatchEventListener) seems unnecessary.

If the target serialization format is Json, it would be beneficial to provide Jackson serialization info in the event types themselves, e.g.:

  public record AfterRefreshTableEvent(
      @JsonProperty("catalog_name")
      String catalogName,
      @JsonProperty("table_identifier")
      TableIdentifier tableIdentifier)
      implements PolarisEvent {}

Or alternatively:

  @JsonNaming(SnakeCaseStrategy.class)
  public record AfterRefreshTableEvent(
      String catalogName,
      TableIdentifier tableIdentifier)
      implements PolarisEvent {}

The above would be naturally serialized as:

{
  "catalog_name" : "test_catalog",
  "table_identifier" : {
    "namespace" : [ "test_namespace" ],
    "name" : "test_table"
  }
}

Without the need for PropertyMapEventListener.

PropertyMapEventListener will also become problematic for larger payloads e.g. TableMetadata as the serialization logic is not well-defined (which fields to include? which case? which order?)

To Reproduce

No response

Actual Behavior

No response

Expected Behavior

No response

Additional context

No response

System information

No response

[vchag]

@adutra I'm happy to take this up.

[adutra]

Thanks @vchag, the issue is assigned to you!

[vchag]

Thanks @adutra ! ... a few thoughts; curious for your take.

1. Currently PropertyMapEventListener the name makes it sound like a generic event listener but it only overrides onAfterRefreshTable, which does look odd. If the intent was to only care about AfterRefreshTableEvent then I think we should rename PropertyMapEventListener to AfterRefreshTableEventListener, any thoughts?

2. Properties parameter HashMap -> Map: This becomes moot if we agree on #3.

3. events -> maps -> JSON: Since the target serialization format is Json, the extra hope is unnecessary. Direct Jackson annotations will couple the event classes to JSON and hinder future Avro/Protobuf support. let’s use Jackson Mixins instead to keep the models serializer agnostic while still producing consistent JSON?

[adutra]

@vchag Indeed I think we need to get rid of PropertyMapEventListener.

I'm not sure that Jackson Annotations on the events themselves would hinder Protobuf or Avro serialization, but I agree that mixins could be a nice alternative. I don't have strong opinions on the "how" as long as we agree on the "why".

Cc @adnanhemani

[vchag]

Well, yes using Jackson Annotation on events themselves won't hinder Protobuf of Avro Serialization. But, it would certainly introduce annotation clutter.

Mixins provide clear separation between Domain login and Serialization logic.