You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "vchag (via GitHub)" <gi...@apache.org> on 2025/10/17 15:06:08 UTC
[PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
vchag opened a new pull request, #2836:
URL: https://github.com/apache/polaris/pull/2836
These changes were created to address concerns raised in Issues #2630
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
### What changes were proposed in this pull request?
1. 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.
2. 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`
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some IRC features, you can provide some references of other IRC implementations / IRC spec.
4. If there is design documentation, please add the link.
5. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Polaris versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
### CHANGELOG.md
<!--
If the changes need to be included in CHANGELOG.md, please add a line here and in CHANGELOG.md.
-->
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2464127959
##########
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:
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"}`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag closed pull request #2836: Introducing AfterRefreshTableEventListener and Mixins
URL: https://github.com/apache/polaris/pull/2836
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2464099543
##########
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:
This was a oversight on my part.... will remove them.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on PR #2836:
URL: https://github.com/apache/polaris/pull/2836#issuecomment-3479081643
Here's the new PR https://github.com/apache/polaris/pull/2962.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2465321613
##########
runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.java:
##########
@@ -19,24 +19,39 @@
package org.apache.polaris.service.events.json.mixins;
-import com.fasterxml.jackson.annotation.JsonValue;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.iceberg.catalog.Namespace;
-/**
- * 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() {}
+/** Mixins for Iceberg classes we don't control, to keep JSON concise. */
+public final class IcebergMixins {
+
+ // Private constructor to prevent instantiation
+ private IcebergMixins() {}
+ /** Serializes Namespace as an object like: "namespace": ["sales", "north.america"] */
public abstract static class NamespaceMixin {
+ @JsonProperty("namespace")
+ public abstract String[] levels();
+
@Override
- @JsonValue
- public abstract String toString(); // serializes "namespace" as "db.sales"
+ @JsonIgnore
+ public abstract String toString();
Review Comment:
Do we still need to declare this method?
##########
runtime/service/src/main/java/org/apache/polaris/service/events/jsonEventListener/AllEventsForwardingListener.java:
##########
@@ -0,0 +1,855 @@
+/*
+ * 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.jsonEventListener;
+
+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.CatalogGenericTableServiceEvents;
+import org.apache.polaris.service.events.CatalogPolicyServiceEvents;
+import org.apache.polaris.service.events.CatalogsServiceEvents;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;
+import org.apache.polaris.service.events.PolarisEvent;
+import org.apache.polaris.service.events.PrincipalRolesServiceEvents;
+import org.apache.polaris.service.events.PrincipalsServiceEvents;
+import org.apache.polaris.service.events.listeners.PolarisEventListener;
+
+/**
+ * Base class for event listeners that with to generically forward all {@link PolarisEvent
+ * PolarisEvents} to an external sinks
Review Comment:
```suggestion
* PolarisEvents} to an external sink.
```
##########
runtime/service/src/main/java/org/apache/polaris/service/config/PolarisAWSCloudWatchObjectMapperProducer.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.config;
+
+import com.fasterxml.jackson.annotation.JsonAutoDetect;
+import com.fasterxml.jackson.annotation.PropertyAccessor;
+import com.fasterxml.jackson.core.StreamReadConstraints;
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.PropertyNamingStrategies;
+import io.quarkus.runtime.configuration.MemorySize;
+import io.quarkus.runtime.configuration.MemorySizeConverter;
+import io.smallrye.config.WithConverter;
+import jakarta.enterprise.inject.Produces;
+import jakarta.inject.Inject;
+import jakarta.inject.Singleton;
+import org.apache.polaris.service.events.PolarisEvent;
+import org.apache.polaris.service.events.json.mixins.IcebergMixins;
+import org.apache.polaris.service.events.json.mixins.PolarisEventBaseMixin;
+import org.eclipse.microprofile.config.inject.ConfigProperty;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** This mapper is isolated and used exclusively for CloudWatch event serializations */
+public class PolarisAWSCloudWatchObjectMapperProducer {
Review Comment:
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.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.java:
##########
@@ -19,24 +19,39 @@
package org.apache.polaris.service.events.json.mixins;
-import com.fasterxml.jackson.annotation.JsonValue;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.iceberg.catalog.Namespace;
-/**
- * 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() {}
+/** Mixins for Iceberg classes we don't control, to keep JSON concise. */
+public final class IcebergMixins {
+
+ // Private constructor to prevent instantiation
+ private IcebergMixins() {}
+ /** Serializes Namespace as an object like: "namespace": ["sales", "north.america"] */
public abstract static class NamespaceMixin {
+ @JsonProperty("namespace")
+ public abstract String[] levels();
+
@Override
- @JsonValue
- public abstract String toString(); // serializes "namespace" as "db.sales"
+ @JsonIgnore
+ public abstract String toString();
}
+ /**
+ * Serializes TableIdentifier as a scalar string like: {"namespace": ["sales", "north.america"],
+ * "name": "transactions"}
+ */
public abstract static class TableIdentifierMixin {
+ @JsonProperty("namespace")
+ public abstract Namespace namespace();
+
+ @JsonProperty("name")
+ public abstract String name();
+
@Override
- @JsonValue
- public abstract String toString(); // serializes "table_identifier" as "db.sales.orders"
+ @JsonIgnore
+ public abstract String toString();
Review Comment:
ditto
##########
site/content/in-dev/unreleased/configuration.md:
##########
@@ -81,52 +81,51 @@ read-only mode, as Polaris only reads the configuration file once, at startup.
## Polaris Configuration Options Reference
-| Configuration Property | Default Value | Description |
-|----------------------------------------------------------------------------------------|------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `polaris.persistence.type` | `relational-jdbc` | Define the persistence backend used by Polaris (`in-memory`, `relational-jdbc`, `eclipse-link` (deprecated)). See [Configuring Apache Polaris for Production)[{{% ref "configuring-polaris-for-production.md" %}}) |
-| `polaris.persistence.relational.jdbc.max-retries` | `1` | Total number of retries JDBC persistence will attempt on connection resets or serialization failures before giving up. |
-| `polaris.persistence.relational.jdbc.max_duaration_in_ms` | `5000 ms` | Max time interval (ms) since the start of a transaction when retries can be attempted. |
-| `polaris.persistence.relational.jdbc.initial_delay_in_ms` | `100 ms` | Initial delay before retrying. The delay is doubled after each retry. |
-| `polaris.persistence.eclipselink.configurationFile` | | Define the location of the `persistence.xml`. By default, it's the built-in `persistence.xml` in use. |
-| `polaris.persistence.eclipselink.persistenceUnit` | `polaris` | Define the name of the persistence unit to use, as defined in the `persistence.xml`. |
-| `polaris.realm-context.type` | `default` | Define the type of the Polaris realm to use. |
-| `polaris.realm-context.realms` | `POLARIS` | Define the list of realms to use. |
-| `polaris.realm-context.header-name` | `Polaris-Realm` | Define the header name defining the realm context. |
-| `polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"` | `false` | Flag to enforce check if credential rotation. |
-| `polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"` | `S3`, `GCS`, `AZURE` | Define the catalog supported storage. Supported values are `S3`, `GCS`, `AZURE`, `FILE`. |
-| `polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"` | `true` | "Override" realm features, here the skip credential subscoping indirection flag. |
-| `polaris.authentication.authenticator.type` | `default` | Define the Polaris authenticator type. |
-| `polaris.authentication.token-service.type` | `default` | Define the Polaris token service type. |
-| `polaris.authentication.token-broker.type` | `rsa-key-pair` | Define the Polaris token broker type. Also configure the location of the key files. For RSA: if the locations of the key files are not configured, an ephemeral key-pair will be created on each Polaris server instance startup, which breaks existing tokens after server restarts and is also incompatible with running multiple Polaris server instances. |
-| `polaris.authentication.token-broker.max-token-generation` | `PT1H` | Define the max token generation policy on the token broker. |
-| `polaris.authentication.token-broker.rsa-key-pair.private-key-file` | | Define the location of the RSA-256 private key file, if present the `public-key` file must be specified, too. |
-| `polaris.authentication.token-broker.rsa-key-pair.public-key-file` | | Define the location of the RSA-256 public key file, if present the `private-key` file must be specified, too. |
-| `polaris.authentication.token-broker.symmetric-key.secret` | `secret` | Define the secret of the symmetric key. |
-| `polaris.authentication.token-broker.symmetric-key.file` | `/tmp/symmetric.key` | Define the location of the symmetric key file. |
-| `polaris.storage.aws.access-key` | `accessKey` | Define the AWS S3 access key. If unset, the default credential provider chain will be used. |
-| `polaris.storage.aws.secret-key` | `secretKey` | Define the AWS S3 secret key. If unset, the default credential provider chain will be used. |
-| `polaris.storage.gcp.token` | `token` | Define the Google Cloud Storage token. If unset, the default credential provider chain will be used. |
-| `polaris.storage.gcp.lifespan` | `PT1H` | Define the Google Cloud Storage lifespan type. If unset, the default credential provider chain will be used. |
-| `polaris.log.request-id-header-name` | `Polaris-Request-Id` | Define the header name to match request ID in the log. |
-| `polaris.log.mdc.aid` | `polaris` | Define the log context (e.g. MDC) AID. |
-| `polaris.log.mdc.sid` | `polaris-service` | Define the log context (e.g. MDC) SID. |
-| `polaris.rate-limiter.filter.type` | `no-op` | Define the Polaris rate limiter. Supported values are `no-op`, `token-bucket`. |
-| `polaris.rate-limiter.token-bucket.type` | `default` | Define the token bucket rate limiter. |
-| `polaris.rate-limiter.token-bucket.requests-per-second` | `9999` | Define the number of requests per second for the token bucket rate limiter. |
-| `polaris.rate-limiter.token-bucket.window` | `PT10S` | Define the window type for the token bucket rate limiter. |
-| `polaris.metrics.tags.<tag-name>=<tag-value>` | `application=Polaris` | Define arbitrary metric tags to include in every request. |
-| `polaris.metrics.realm-id-tag.api-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in API metrics. |
-| `polaris.metrics.realm-id-tag.http-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in HTTP request metrics. |
-| `polaris.metrics.realm-id-tag.http-metrics-max-cardinality` | `100` | The maximum cardinality for the `realm_id` tag in HTTP request metrics. |
-| `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`. |
+| Configuration Property | Default Value | Description |
+|----------------------------------------------------------------------------------------|----------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `polaris.persistence.type` | `relational-jdbc` | Define the persistence backend used by Polaris (`in-memory`, `relational-jdbc`, `eclipse-link` (deprecated)). See [Configuring Apache Polaris for Production)[{{% ref "configuring-polaris-for-production.md" %}}) |
+| `polaris.persistence.relational.jdbc.max-retries` | `1` | Total number of retries JDBC persistence will attempt on connection resets or serialization failures before giving up. |
+| `polaris.persistence.relational.jdbc.max_duaration_in_ms` | `5000 ms` | Max time interval (ms) since the start of a transaction when retries can be attempted. |
+| `polaris.persistence.relational.jdbc.initial_delay_in_ms` | `100 ms` | Initial delay before retrying. The delay is doubled after each retry. |
+| `polaris.persistence.eclipselink.configurationFile` | | Define the location of the `persistence.xml`. By default, it's the built-in `persistence.xml` in use. |
+| `polaris.persistence.eclipselink.persistenceUnit` | `polaris` | Define the name of the persistence unit to use, as defined in the `persistence.xml`. |
+| `polaris.realm-context.type` | `default` | Define the type of the Polaris realm to use. |
+| `polaris.realm-context.realms` | `POLARIS` | Define the list of realms to use. |
+| `polaris.realm-context.header-name` | `Polaris-Realm` | Define the header name defining the realm context. |
+| `polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"` | `false` | Flag to enforce check if credential rotation. |
+| `polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"` | `S3`, `GCS`, `AZURE` | Define the catalog supported storage. Supported values are `S3`, `GCS`, `AZURE`, `FILE`. |
+| `polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"` | `true` | "Override" realm features, here the skip credential subscoping indirection flag. |
+| `polaris.authentication.authenticator.type` | `default` | Define the Polaris authenticator type. |
+| `polaris.authentication.token-service.type` | `default` | Define the Polaris token service type. |
+| `polaris.authentication.token-broker.type` | `rsa-key-pair` | Define the Polaris token broker type. Also configure the location of the key files. For RSA: if the locations of the key files are not configured, an ephemeral key-pair will be created on each Polaris server instance startup, which breaks existing tokens after server restarts and is also incompatible with running multiple Polaris server instances. |
+| `polaris.authentication.token-broker.max-token-generation` | `PT1H` | Define the max token generation policy on the token broker. |
+| `polaris.authentication.token-broker.rsa-key-pair.private-key-file` | | Define the location of the RSA-256 private key file, if present the `public-key` file must be specified, too. |
+| `polaris.authentication.token-broker.rsa-key-pair.public-key-file` | | Define the location of the RSA-256 public key file, if present the `private-key` file must be specified, too. |
+| `polaris.authentication.token-broker.symmetric-key.secret` | `secret` | Define the secret of the symmetric key. |
+| `polaris.authentication.token-broker.symmetric-key.file` | `/tmp/symmetric.key` | Define the location of the symmetric key file. |
+| `polaris.storage.aws.access-key` | `accessKey` | Define the AWS S3 access key. If unset, the default credential provider chain will be used. |
+| `polaris.storage.aws.secret-key` | `secretKey` | Define the AWS S3 secret key. If unset, the default credential provider chain will be used. |
+| `polaris.storage.gcp.token` | `token` | Define the Google Cloud Storage token. If unset, the default credential provider chain will be used. |
+| `polaris.storage.gcp.lifespan` | `PT1H` | Define the Google Cloud Storage lifespan type. If unset, the default credential provider chain will be used. |
+| `polaris.log.request-id-header-name` | `Polaris-Request-Id` | Define the header name to match request ID in the log. |
+| `polaris.log.mdc.aid` | `polaris` | Define the log context (e.g. MDC) AID. |
+| `polaris.log.mdc.sid` | `polaris-service` | Define the log context (e.g. MDC) SID. |
+| `polaris.rate-limiter.filter.type` | `no-op` | Define the Polaris rate limiter. Supported values are `no-op`, `token-bucket`. |
+| `polaris.rate-limiter.token-bucket.type` | `default` | Define the token bucket rate limiter. |
+| `polaris.rate-limiter.token-bucket.requests-per-second` | `9999` | Define the number of requests per second for the token bucket rate limiter. |
+| `polaris.rate-limiter.token-bucket.window` | `PT10S` | Define the window type for the token bucket rate limiter. |
+| `polaris.metrics.tags.<tag-name>=<tag-value>` | `application=Polaris` | Define arbitrary metric tags to include in every request. |
+| `polaris.metrics.realm-id-tag.api-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in API metrics. |
+| `polaris.metrics.realm-id-tag.http-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in HTTP request metrics. |
+| `polaris.metrics.realm-id-tag.http-metrics-max-cardinality` | `100` | The maximum cardinality for the `realm_id` tag in HTTP request metrics. |
+| `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`. |
| `polaris.event-listener.aws-cloudwatch.log-group` | `polaris-cloudwatch-default-group` | Define the AWS CloudWatch log group name for the event listener. |
| `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. |
-
Review Comment:
Nit: Why remove this line?
##########
runtime/service/src/main/java/org/apache/polaris/service/quarkus/events/jsonEventListener/aws/cloudwatch/QuarkusAwsCloudWatchConfiguration.java:
##########
@@ -96,4 +97,21 @@ public interface QuarkusAwsCloudWatchConfiguration extends AwsCloudWatchConfigur
@WithDefault("false")
@Override
boolean synchronousMode();
+
+ /**
+ * Fully-qualified event type names to forward.
+ *
+ * <p>Empty list -> handle ALL events. Example in application.properties:
+ * polaris.event-listener.aws-cloudwatch.event-types=\
+ * org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent,\
+ * org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterCommitTableEvent
+ *
+ * <p>Configuration property: {@code polaris.event-listener.aws-cloudwatch.event-types}
+ *
+ * @return a set of event types
+ */
+ @WithName("event-types")
+ @WithDefault("org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent")
Review Comment:
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:
https://github.com/apache/polaris/blob/0acbb64692e6506967d3fee6a4e2f35266ae8138/runtime/defaults/src/main/resources/application.properties#L142-L147
##########
runtime/service/src/main/java/org/apache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java:
##########
@@ -77,14 +78,28 @@ public class AwsCloudWatchEventListener extends AfterRefreshTableEventListener {
public AwsCloudWatchEventListener(
AwsCloudWatchConfiguration config,
Clock clock,
- PolarisIcebergObjectMapperCustomizer customizer) {
+ @PolarisAWSCloudWatchObjectMapper ObjectMapper mapper) {
this.logStream = config.awsCloudWatchLogStream();
this.logGroup = config.awsCloudWatchLogGroup();
this.region = Region.of(config.awsCloudWatchRegion());
this.synchronousMode = config.synchronousMode();
this.clock = clock;
- this.objectMapper = ObjectMapperFactory.create();
- customizer.customize(this.objectMapper);
+ this.objectMapper = mapper;
+ this.allowedEventTypes =
+ config.eventTypes().stream()
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toUnmodifiableSet());
+ }
+
+ @Override
+ protected boolean shouldHandle(Object event) {
Review Comment:
Nice, this is exactly what I had in mind ๐
##########
runtime/service/src/main/java/org/apache/polaris/service/quarkus/events/jsonEventListener/aws/cloudwatch/QuarkusAwsCloudWatchConfiguration.java:
##########
@@ -96,4 +97,21 @@ public interface QuarkusAwsCloudWatchConfiguration extends AwsCloudWatchConfigur
@WithDefault("false")
@Override
boolean synchronousMode();
+
+ /**
+ * Fully-qualified event type names to forward.
+ *
+ * <p>Empty list -> handle ALL events. Example in application.properties:
+ * polaris.event-listener.aws-cloudwatch.event-types=\
+ * org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent,\
+ * org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterCommitTableEvent
+ *
+ * <p>Configuration property: {@code polaris.event-listener.aws-cloudwatch.event-types}
+ *
+ * @return a set of event types
+ */
+ @WithName("event-types")
+ @WithDefault("org.apache.polaris.service.events.IcebergRestCatalogEvents.AfterRefreshTableEvent")
+ @Override
+ Set<String> eventTypes();
Review Comment:
I think this could be `Set<Class<? extends PolarisEvent>>`.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/json/mixins/IcebergMixins.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:
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.
##########
runtime/service/src/main/java/org/apache/polaris/service/config/PolarisAWSCloudWatchObjectMapperProducer.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.config;
+
+import com.fasterxml.jackson.annotation.JsonAutoDetect;
+import com.fasterxml.jackson.annotation.PropertyAccessor;
+import com.fasterxml.jackson.core.StreamReadConstraints;
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.PropertyNamingStrategies;
+import io.quarkus.runtime.configuration.MemorySize;
+import io.quarkus.runtime.configuration.MemorySizeConverter;
+import io.smallrye.config.WithConverter;
+import jakarta.enterprise.inject.Produces;
+import jakarta.inject.Inject;
+import jakarta.inject.Singleton;
+import org.apache.polaris.service.events.PolarisEvent;
+import org.apache.polaris.service.events.json.mixins.IcebergMixins;
+import org.apache.polaris.service.events.json.mixins.PolarisEventBaseMixin;
+import org.eclipse.microprofile.config.inject.ConfigProperty;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** This mapper is isolated and used exclusively for CloudWatch event serializations */
+public class PolarisAWSCloudWatchObjectMapperProducer {
+ private static final Logger LOGGER =
+ LoggerFactory.getLogger(PolarisAWSCloudWatchObjectMapperProducer.class);
+
+ private final long maxBodySize;
+
+ @Inject
+ public PolarisAWSCloudWatchObjectMapperProducer(
+ @ConfigProperty(name = "polaris.aws.cloudwatch.max-body-size", defaultValue = "16M")
Review Comment:
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.`.
##########
site/content/in-dev/unreleased/configuration.md:
##########
@@ -81,52 +81,51 @@ read-only mode, as Polaris only reads the configuration file once, at startup.
## Polaris Configuration Options Reference
-| Configuration Property | Default Value | Description |
-|----------------------------------------------------------------------------------------|------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `polaris.persistence.type` | `relational-jdbc` | Define the persistence backend used by Polaris (`in-memory`, `relational-jdbc`, `eclipse-link` (deprecated)). See [Configuring Apache Polaris for Production)[{{% ref "configuring-polaris-for-production.md" %}}) |
-| `polaris.persistence.relational.jdbc.max-retries` | `1` | Total number of retries JDBC persistence will attempt on connection resets or serialization failures before giving up. |
-| `polaris.persistence.relational.jdbc.max_duaration_in_ms` | `5000 ms` | Max time interval (ms) since the start of a transaction when retries can be attempted. |
-| `polaris.persistence.relational.jdbc.initial_delay_in_ms` | `100 ms` | Initial delay before retrying. The delay is doubled after each retry. |
-| `polaris.persistence.eclipselink.configurationFile` | | Define the location of the `persistence.xml`. By default, it's the built-in `persistence.xml` in use. |
-| `polaris.persistence.eclipselink.persistenceUnit` | `polaris` | Define the name of the persistence unit to use, as defined in the `persistence.xml`. |
-| `polaris.realm-context.type` | `default` | Define the type of the Polaris realm to use. |
-| `polaris.realm-context.realms` | `POLARIS` | Define the list of realms to use. |
-| `polaris.realm-context.header-name` | `Polaris-Realm` | Define the header name defining the realm context. |
-| `polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"` | `false` | Flag to enforce check if credential rotation. |
-| `polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"` | `S3`, `GCS`, `AZURE` | Define the catalog supported storage. Supported values are `S3`, `GCS`, `AZURE`, `FILE`. |
-| `polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"` | `true` | "Override" realm features, here the skip credential subscoping indirection flag. |
-| `polaris.authentication.authenticator.type` | `default` | Define the Polaris authenticator type. |
-| `polaris.authentication.token-service.type` | `default` | Define the Polaris token service type. |
-| `polaris.authentication.token-broker.type` | `rsa-key-pair` | Define the Polaris token broker type. Also configure the location of the key files. For RSA: if the locations of the key files are not configured, an ephemeral key-pair will be created on each Polaris server instance startup, which breaks existing tokens after server restarts and is also incompatible with running multiple Polaris server instances. |
-| `polaris.authentication.token-broker.max-token-generation` | `PT1H` | Define the max token generation policy on the token broker. |
-| `polaris.authentication.token-broker.rsa-key-pair.private-key-file` | | Define the location of the RSA-256 private key file, if present the `public-key` file must be specified, too. |
-| `polaris.authentication.token-broker.rsa-key-pair.public-key-file` | | Define the location of the RSA-256 public key file, if present the `private-key` file must be specified, too. |
-| `polaris.authentication.token-broker.symmetric-key.secret` | `secret` | Define the secret of the symmetric key. |
-| `polaris.authentication.token-broker.symmetric-key.file` | `/tmp/symmetric.key` | Define the location of the symmetric key file. |
-| `polaris.storage.aws.access-key` | `accessKey` | Define the AWS S3 access key. If unset, the default credential provider chain will be used. |
-| `polaris.storage.aws.secret-key` | `secretKey` | Define the AWS S3 secret key. If unset, the default credential provider chain will be used. |
-| `polaris.storage.gcp.token` | `token` | Define the Google Cloud Storage token. If unset, the default credential provider chain will be used. |
-| `polaris.storage.gcp.lifespan` | `PT1H` | Define the Google Cloud Storage lifespan type. If unset, the default credential provider chain will be used. |
-| `polaris.log.request-id-header-name` | `Polaris-Request-Id` | Define the header name to match request ID in the log. |
-| `polaris.log.mdc.aid` | `polaris` | Define the log context (e.g. MDC) AID. |
-| `polaris.log.mdc.sid` | `polaris-service` | Define the log context (e.g. MDC) SID. |
-| `polaris.rate-limiter.filter.type` | `no-op` | Define the Polaris rate limiter. Supported values are `no-op`, `token-bucket`. |
-| `polaris.rate-limiter.token-bucket.type` | `default` | Define the token bucket rate limiter. |
-| `polaris.rate-limiter.token-bucket.requests-per-second` | `9999` | Define the number of requests per second for the token bucket rate limiter. |
-| `polaris.rate-limiter.token-bucket.window` | `PT10S` | Define the window type for the token bucket rate limiter. |
-| `polaris.metrics.tags.<tag-name>=<tag-value>` | `application=Polaris` | Define arbitrary metric tags to include in every request. |
-| `polaris.metrics.realm-id-tag.api-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in API metrics. |
-| `polaris.metrics.realm-id-tag.http-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in HTTP request metrics. |
-| `polaris.metrics.realm-id-tag.http-metrics-max-cardinality` | `100` | The maximum cardinality for the `realm_id` tag in HTTP request metrics. |
-| `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`. |
+| Configuration Property | Default Value | Description |
+|----------------------------------------------------------------------------------------|----------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `polaris.persistence.type` | `relational-jdbc` | Define the persistence backend used by Polaris (`in-memory`, `relational-jdbc`, `eclipse-link` (deprecated)). See [Configuring Apache Polaris for Production)[{{% ref "configuring-polaris-for-production.md" %}}) |
+| `polaris.persistence.relational.jdbc.max-retries` | `1` | Total number of retries JDBC persistence will attempt on connection resets or serialization failures before giving up. |
+| `polaris.persistence.relational.jdbc.max_duaration_in_ms` | `5000 ms` | Max time interval (ms) since the start of a transaction when retries can be attempted. |
+| `polaris.persistence.relational.jdbc.initial_delay_in_ms` | `100 ms` | Initial delay before retrying. The delay is doubled after each retry. |
+| `polaris.persistence.eclipselink.configurationFile` | | Define the location of the `persistence.xml`. By default, it's the built-in `persistence.xml` in use. |
+| `polaris.persistence.eclipselink.persistenceUnit` | `polaris` | Define the name of the persistence unit to use, as defined in the `persistence.xml`. |
+| `polaris.realm-context.type` | `default` | Define the type of the Polaris realm to use. |
+| `polaris.realm-context.realms` | `POLARIS` | Define the list of realms to use. |
+| `polaris.realm-context.header-name` | `Polaris-Realm` | Define the header name defining the realm context. |
+| `polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"` | `false` | Flag to enforce check if credential rotation. |
+| `polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"` | `S3`, `GCS`, `AZURE` | Define the catalog supported storage. Supported values are `S3`, `GCS`, `AZURE`, `FILE`. |
+| `polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"` | `true` | "Override" realm features, here the skip credential subscoping indirection flag. |
+| `polaris.authentication.authenticator.type` | `default` | Define the Polaris authenticator type. |
+| `polaris.authentication.token-service.type` | `default` | Define the Polaris token service type. |
+| `polaris.authentication.token-broker.type` | `rsa-key-pair` | Define the Polaris token broker type. Also configure the location of the key files. For RSA: if the locations of the key files are not configured, an ephemeral key-pair will be created on each Polaris server instance startup, which breaks existing tokens after server restarts and is also incompatible with running multiple Polaris server instances. |
+| `polaris.authentication.token-broker.max-token-generation` | `PT1H` | Define the max token generation policy on the token broker. |
+| `polaris.authentication.token-broker.rsa-key-pair.private-key-file` | | Define the location of the RSA-256 private key file, if present the `public-key` file must be specified, too. |
+| `polaris.authentication.token-broker.rsa-key-pair.public-key-file` | | Define the location of the RSA-256 public key file, if present the `private-key` file must be specified, too. |
+| `polaris.authentication.token-broker.symmetric-key.secret` | `secret` | Define the secret of the symmetric key. |
+| `polaris.authentication.token-broker.symmetric-key.file` | `/tmp/symmetric.key` | Define the location of the symmetric key file. |
+| `polaris.storage.aws.access-key` | `accessKey` | Define the AWS S3 access key. If unset, the default credential provider chain will be used. |
+| `polaris.storage.aws.secret-key` | `secretKey` | Define the AWS S3 secret key. If unset, the default credential provider chain will be used. |
+| `polaris.storage.gcp.token` | `token` | Define the Google Cloud Storage token. If unset, the default credential provider chain will be used. |
+| `polaris.storage.gcp.lifespan` | `PT1H` | Define the Google Cloud Storage lifespan type. If unset, the default credential provider chain will be used. |
+| `polaris.log.request-id-header-name` | `Polaris-Request-Id` | Define the header name to match request ID in the log. |
+| `polaris.log.mdc.aid` | `polaris` | Define the log context (e.g. MDC) AID. |
+| `polaris.log.mdc.sid` | `polaris-service` | Define the log context (e.g. MDC) SID. |
+| `polaris.rate-limiter.filter.type` | `no-op` | Define the Polaris rate limiter. Supported values are `no-op`, `token-bucket`. |
+| `polaris.rate-limiter.token-bucket.type` | `default` | Define the token bucket rate limiter. |
+| `polaris.rate-limiter.token-bucket.requests-per-second` | `9999` | Define the number of requests per second for the token bucket rate limiter. |
+| `polaris.rate-limiter.token-bucket.window` | `PT10S` | Define the window type for the token bucket rate limiter. |
+| `polaris.metrics.tags.<tag-name>=<tag-value>` | `application=Polaris` | Define arbitrary metric tags to include in every request. |
+| `polaris.metrics.realm-id-tag.api-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in API metrics. |
+| `polaris.metrics.realm-id-tag.http-metrics-enabled` | `false` | Whether to enable the `realm_id` metric tag in HTTP request metrics. |
+| `polaris.metrics.realm-id-tag.http-metrics-max-cardinality` | `100` | The maximum cardinality for the `realm_id` tag in HTTP request metrics. |
+| `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`. |
Review Comment:
BTW `persistence-in-memory-buffer` is also supported.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2836:
URL: https://github.com/apache/polaris/pull/2836#issuecomment-3468123008
@vchag what's the status here? Are you still able to work in this issue?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2448136669
##########
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`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2462348615
##########
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:
This was an oversight on my part. I'll remove it.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2462343097
##########
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:
This is redundant. I'll go ahead and remove it.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2462348615
##########
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:
This was an oversight on my part. I'll remove it.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2462342041
##########
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:
Will rename.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2464087939
##########
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:
Actually, Yes. We don't need this mixin.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on PR #2836:
URL: https://github.com/apache/polaris/pull/2836#issuecomment-3478755469
@adutra Yes, I'm on it. Will release it shortly in the next 4 - 8 hrs.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2464203127
##########
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:
Oh right. Lets introduce a new config for `aws-cloudwatch.event-types`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2464099543
##########
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:
This was an oversight on my part.... will remove them.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2464099067
##########
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:
I agree with the CDI based approach. I found option #2 to be better feasible.
Let's seize the opportunity!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on code in PR #2836:
URL: https://github.com/apache/polaris/pull/2836#discussion_r2462347826
##########
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:
How about we re-name it `listener`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introducing AfterRefreshTableEventListener and Mixins [polaris]
Posted by "vchag (via GitHub)" <gi...@apache.org>.
vchag commented on PR #2836:
URL: https://github.com/apache/polaris/pull/2836#issuecomment-3454365874
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org