adutra commented on code in PR #2836: URL: https://github.com/apache/polaris/pull/2836#discussion_r2447942962
########## runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/ObjectMapperFactory.java: ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.service.events.json.mixins; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.datatype.jdk8.Jdk8Module; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.polaris.service.events.IcebergRestCatalogEvents; +import org.apache.polaris.service.events.PolarisEvent; + +public class ObjectMapperFactory { + private ObjectMapperFactory() {} + + public static ObjectMapper create() { Review Comment: You should not create an ObjectMapper like this, but rather use CDI. 1. 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. 2. 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. ########## runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergThirdPartyMixins.java: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.service.events.json.mixins; + +import com.fasterxml.jackson.annotation.JsonValue; + +/** + * Mixins for Iceberg classes we don't control, to keep JSON concise. The @JsonValue marks + * toString() as the value to serialize. + */ +public class IcebergThirdPartyMixins { + private IcebergThirdPartyMixins() {} + + public abstract static class NamespaceMixin { Review Comment: 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: ```java 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. ########## runtime/service/src/main/java/org/apache/polaris/service/events/json/serde/TableIdentifierToStringSerializer.java: ########## @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.service.events.json.serde; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.JsonSerializer; +import com.fasterxml.jackson.databind.SerializerProvider; +import java.io.IOException; +import org.apache.iceberg.catalog.TableIdentifier; + +public class TableIdentifierToStringSerializer extends JsonSerializer<TableIdentifier> { Review Comment: Why do we need this serializer? We already have `IcebergThirdPartyMixins.TableIdentifierMixin`. ########## runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/ObjectMapperFactory.java: ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.service.events.json.mixins; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; +import com.fasterxml.jackson.datatype.jdk8.Jdk8Module; +import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.polaris.service.events.IcebergRestCatalogEvents; +import org.apache.polaris.service.events.PolarisEvent; + +public class ObjectMapperFactory { + private ObjectMapperFactory() {} + + public static ObjectMapper create() { + ObjectMapper mapper = + new ObjectMapper() + .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) Review Comment: Can you explain why we need to disable these features? I would prefer to customize the mapper as little as possible. ########## runtime/service/src/main/java/org/apache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java: ########## @@ -151,28 +158,44 @@ void shutdown() { } } + @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) + public record CloudWatchEvent( + String principal, + String realmId, + Collection<String> activatedRoles, + String eventType, + @JsonUnwrapped PolarisEvent event // flatten + ) {} + @Override - protected void transformAndSendEvent(HashMap<String, Object> properties) { - properties.put("realm_id", callContext.getRealmContext().getRealmIdentifier()); - properties.put("principal", securityContext.getUserPrincipal().getName()); - properties.put( - "activated_roles", ((PolarisPrincipal) securityContext.getUserPrincipal()).getRoles()); - // TODO: Add request ID when it is available + protected void transformAndSendEvent(IcebergRestCatalogEvents.AfterRefreshTableEvent event) { Review Comment: 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: ```properties 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`. ########## runtime/service/src/main/java/org/apache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java: ########## @@ -151,28 +158,44 @@ void shutdown() { } } + @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) + public record CloudWatchEvent( + String principal, + String realmId, + Collection<String> activatedRoles, + String eventType, + @JsonUnwrapped PolarisEvent event // flatten Review Comment: Nice 👍 ########## runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergThirdPartyMixins.java: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.service.events.json.mixins; + +import com.fasterxml.jackson.annotation.JsonValue; + +/** + * Mixins for Iceberg classes we don't control, to keep JSON concise. The @JsonValue marks + * toString() as the value to serialize. + */ +public class IcebergThirdPartyMixins { Review Comment: nit: rename to `IcebergMixins` and make it final. ########## runtime/service/src/main/java/org/apache/polaris/service/events/jsonEventListener/AfterRefreshTableEventListener.java: ########## @@ -19,7 +19,6 @@ package org.apache.polaris.service.events.jsonEventListener; Review Comment: 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 ########## runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/AfterRefreshTableEventMixin.java: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.polaris.service.events.json.mixins; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeName; +import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.polaris.service.events.json.serde.TableIdentifierToStringSerializer; + +@JsonTypeName("AfterRefreshTableEvent") +public abstract class AfterRefreshTableEventMixin { Review Comment: I don't think you need this mixin, we already have specified `@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)` in `PolarisEventBaseMixin`. That should be enough. ########## runtime/service/src/main/java/org/apache/polaris/service/events/jsonEventListener/AfterRefreshTableEventListener.java: ########## @@ -30,14 +29,12 @@ * {{@code @link#transformAndSendEvent(HashMap)}} method to define how the event data should be * transformed into a JSON string, transmitted, and/or stored. */ -public abstract class PropertyMapEventListener implements PolarisEventListener { - protected abstract void transformAndSendEvent(HashMap<String, Object> properties); +public abstract class AfterRefreshTableEventListener implements PolarisEventListener { Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
