You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adnanhemani (via GitHub)" <gi...@apache.org> on 2025/08/31 06:16:53 UTC
[PR] Add Events for Iceberg REST APIs [polaris]
adnanhemani opened a new pull request, #2480:
URL: https://github.com/apache/polaris/pull/2480
<!--
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.
-->
This PR adds the Events instrumentation for the Iceberg REST Service APIs, surrounding the default delegated call to the business logic APIs.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2331534209
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -103,8 +202,29 @@ public Response createTable(
String accessDelegationMode,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.createTable(
- prefix, namespace, createTableRequest, accessDelegationMode, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ if (!createTableRequest.stageCreate()) {
Review Comment:
I did not realize this - have removed the flag around BeforeCreateTableEvent. Consumers of this event should see this is for a `StageCreate` table and surface that information accordingly.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2321681626
##########
runtime/service/src/main/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListener.java:
##########
@@ -20,61 +20,55 @@
package org.apache.polaris.service.events.listeners;
import java.util.Map;
+import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.polaris.core.entity.PolarisEvent;
-import org.apache.polaris.service.events.AfterCatalogCreatedEvent;
-import org.apache.polaris.service.events.AfterTableCommitedEvent;
-import org.apache.polaris.service.events.AfterTableCreatedEvent;
-import org.apache.polaris.service.events.AfterTableRefreshedEvent;
-import org.apache.polaris.service.events.AfterTaskAttemptedEvent;
-import org.apache.polaris.service.events.AfterViewCommitedEvent;
-import org.apache.polaris.service.events.AfterViewRefreshedEvent;
-import org.apache.polaris.service.events.BeforeRequestRateLimitedEvent;
-import org.apache.polaris.service.events.BeforeTableCommitedEvent;
-import org.apache.polaris.service.events.BeforeTableRefreshedEvent;
-import org.apache.polaris.service.events.BeforeTaskAttemptedEvent;
-import org.apache.polaris.service.events.BeforeViewCommitedEvent;
-import org.apache.polaris.service.events.BeforeViewRefreshedEvent;
+import org.apache.polaris.service.events.AfterAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeLimitRequestRateEvent;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;
public abstract class PolarisPersistenceEventListener extends PolarisEventListener {
// TODO: Ensure all events (except RateLimiter ones) call `addToBuffer`
Review Comment:
BTW the comment is outdated:
```suggestion
// TODO: Ensure all events (except RateLimiter ones) call `processEvent`
```
##########
runtime/service/src/main/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListener.java:
##########
@@ -20,61 +20,55 @@
package org.apache.polaris.service.events.listeners;
import java.util.Map;
+import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.polaris.core.entity.PolarisEvent;
-import org.apache.polaris.service.events.AfterCatalogCreatedEvent;
-import org.apache.polaris.service.events.AfterTableCommitedEvent;
-import org.apache.polaris.service.events.AfterTableCreatedEvent;
-import org.apache.polaris.service.events.AfterTableRefreshedEvent;
-import org.apache.polaris.service.events.AfterTaskAttemptedEvent;
-import org.apache.polaris.service.events.AfterViewCommitedEvent;
-import org.apache.polaris.service.events.AfterViewRefreshedEvent;
-import org.apache.polaris.service.events.BeforeRequestRateLimitedEvent;
-import org.apache.polaris.service.events.BeforeTableCommitedEvent;
-import org.apache.polaris.service.events.BeforeTableRefreshedEvent;
-import org.apache.polaris.service.events.BeforeTaskAttemptedEvent;
-import org.apache.polaris.service.events.BeforeViewCommitedEvent;
-import org.apache.polaris.service.events.BeforeViewRefreshedEvent;
+import org.apache.polaris.service.events.AfterAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeLimitRequestRateEvent;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;
public abstract class PolarisPersistenceEventListener extends PolarisEventListener {
// TODO: Ensure all events (except RateLimiter ones) call `addToBuffer`
Review Comment:
Is the plan to tackle this later?
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2312280382
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -103,8 +192,23 @@ public Response createTable(
String accessDelegationMode,
RealmContext realmContext,
SecurityContext securityContext) {
Review Comment:
Remove instrumentation from https://github.com/apache/polaris/pull/1844 for this.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2320481928
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -777,8 +777,8 @@ public Response sendNotification(
securityContext,
prefix,
catalog -> {
- catalog.sendNotification(tableIdentifier, notificationRequest);
- return Response.status(Response.Status.NO_CONTENT).build();
+ boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);
Review Comment:
Needing an OpenAPI spec change and ML thread regarding this is a fair ask. Let me revert this out for now - maybe we can proceed without the result of the `sendNotification` call first and then in parallel, I can start the ML thread for this.
Once we reach a consensus as a community, then we can go back and resolve the TODO.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -307,7 +519,18 @@ public Response sendNotification(
NotificationRequest notificationRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.sendNotification(
- prefix, namespace, table, notificationRequest, realmContext, securityContext);
+ String catalogName = getCatalogFromPrefix(prefix, realmContext);
+ polarisEventListener.onBeforeSendNotification(
+ new BeforeSendNotificationEvent(catalogName, namespace, table, notificationRequest));
+ Response resp =
+ delegate.sendNotification(
+ prefix, namespace, table, notificationRequest, realmContext, securityContext);
+ polarisEventListener.onAfterSendNotification(
+ new AfterSendNotificationEvent(catalogName, namespace, table, (boolean) resp.getEntity()));
+ return resp;
+ }
+
+ private String getCatalogFromPrefix(String prefix, RealmContext realmContext) {
+ return prefixParser.prefixToCatalogName(realmContext, prefix);
Review Comment:
Changed.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -103,8 +192,23 @@ public Response createTable(
String accessDelegationMode,
RealmContext realmContext,
SecurityContext securityContext) {
Review Comment:
Done as part of merge from main.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java:
##########
@@ -55,4 +55,162 @@ public void onBeforeTaskAttempted(BeforeTaskAttemptedEvent event) {}
/** {@link AfterTaskAttemptedEvent} */
public void onAfterTaskAttempted(AfterTaskAttemptedEvent event) {}
+
+ // Iceberg REST Catalog Namespace Events
+ /** {@link IcebergRestCatalogEvents.BeforeCreateNamespaceEvent} */
+ public void onBeforeCreateNamespace(IcebergRestCatalogEvents.BeforeCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCreateNamespaceEvent} */
+ public void onAfterCreateNamespace(IcebergRestCatalogEvents.AfterCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeListNamespacesEvent} */
+ public void onBeforeListNamespaces(IcebergRestCatalogEvents.BeforeListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterListNamespacesEvent} */
+ public void onAfterListNamespaces(IcebergRestCatalogEvents.AfterListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent} */
+ public void onBeforeLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent} */
+ public void onAfterLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent} */
+ public void onBeforeCheckNamespaceExists(
+ IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent} */
+ public void onAfterCheckNamespaceExists(
+ IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeDropNamespaceEvent} */
+ public void onBeforeDropNamespace(IcebergRestCatalogEvents.BeforeDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterDropNamespaceEvent} */
+ public void onAfterDropNamespace(IcebergRestCatalogEvents.AfterDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent} */
+ public void onBeforeUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent} */
+ public void onAfterUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent event) {}
+
+ // Iceberg REST Catalog Table Events
Review Comment:
Changed the remaining to be verb-object as well. Pleas let me know if something still stands out :)
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2326037633
##########
runtime/service/src/main/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListener.java:
##########
@@ -20,61 +20,55 @@
package org.apache.polaris.service.events.listeners;
import java.util.Map;
+import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
+import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.polaris.core.entity.PolarisEvent;
-import org.apache.polaris.service.events.AfterCatalogCreatedEvent;
-import org.apache.polaris.service.events.AfterTableCommitedEvent;
-import org.apache.polaris.service.events.AfterTableCreatedEvent;
-import org.apache.polaris.service.events.AfterTableRefreshedEvent;
-import org.apache.polaris.service.events.AfterTaskAttemptedEvent;
-import org.apache.polaris.service.events.AfterViewCommitedEvent;
-import org.apache.polaris.service.events.AfterViewRefreshedEvent;
-import org.apache.polaris.service.events.BeforeRequestRateLimitedEvent;
-import org.apache.polaris.service.events.BeforeTableCommitedEvent;
-import org.apache.polaris.service.events.BeforeTableRefreshedEvent;
-import org.apache.polaris.service.events.BeforeTaskAttemptedEvent;
-import org.apache.polaris.service.events.BeforeViewCommitedEvent;
-import org.apache.polaris.service.events.BeforeViewRefreshedEvent;
+import org.apache.polaris.service.events.AfterAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeAttemptTaskEvent;
+import org.apache.polaris.service.events.BeforeLimitRequestRateEvent;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;
public abstract class PolarisPersistenceEventListener extends PolarisEventListener {
// TODO: Ensure all events (except RateLimiter ones) call `addToBuffer`
Review Comment:
Yes, the plan is to tackle in a separate PR - I would like to keep the focus of this PR on the events itself and the transformations will go in a further PR.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2314233501
##########
runtime/service/src/main/java/org/apache/polaris/service/events/IcebergRestCatalogEvents.java:
##########
@@ -0,0 +1,179 @@
+/*
+ * 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;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
+import org.apache.iceberg.rest.requests.CreateTableRequest;
+import org.apache.iceberg.rest.requests.CreateViewRequest;
+import org.apache.iceberg.rest.requests.RegisterTableRequest;
+import org.apache.iceberg.rest.requests.RenameTableRequest;
+import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest;
+import org.apache.iceberg.rest.responses.ConfigResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.rest.responses.LoadViewResponse;
+import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse;
+import org.apache.polaris.service.types.CommitTableRequest;
+import org.apache.polaris.service.types.CommitViewRequest;
+import org.apache.polaris.service.types.NotificationRequest;
+
+/**
+ * Event records for Iceberg REST Catalog operations. Each operation has corresponding "Before" and
+ * "After" event records.
+ */
+public class IcebergRestCatalogEvents {
Review Comment:
Shouldn't we move here the existing events? E.g. `AfterTableCommitedEvent` etc.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -62,26 +124,45 @@ public Response listNamespaces(
String parent,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.listNamespaces(
- prefix, pageToken, pageSize, parent, realmContext, securityContext);
+ polarisEventListener.onBeforeListNamespaces(new BeforeListNamespacesEvent(prefix, parent));
+ Response resp =
+ delegate.listNamespaces(prefix, pageToken, pageSize, parent, realmContext, securityContext);
+ polarisEventListener.onAfterListNamespaces(new AfterListNamespacesEvent(prefix, parent));
Review Comment:
Sorry if this has been discussed already, but what is the plan for failed events, should we wrap the decorated call in a try-catch block and fire a failed event?
##########
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java:
##########
@@ -55,4 +55,162 @@ public void onBeforeTaskAttempted(BeforeTaskAttemptedEvent event) {}
/** {@link AfterTaskAttemptedEvent} */
public void onAfterTaskAttempted(AfterTaskAttemptedEvent event) {}
+
+ // Iceberg REST Catalog Namespace Events
+ /** {@link IcebergRestCatalogEvents.BeforeCreateNamespaceEvent} */
+ public void onBeforeCreateNamespace(IcebergRestCatalogEvents.BeforeCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCreateNamespaceEvent} */
+ public void onAfterCreateNamespace(IcebergRestCatalogEvents.AfterCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeListNamespacesEvent} */
+ public void onBeforeListNamespaces(IcebergRestCatalogEvents.BeforeListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterListNamespacesEvent} */
+ public void onAfterListNamespaces(IcebergRestCatalogEvents.AfterListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent} */
+ public void onBeforeLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent} */
+ public void onAfterLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent} */
+ public void onBeforeCheckNamespaceExists(
+ IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent} */
+ public void onAfterCheckNamespaceExists(
+ IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeDropNamespaceEvent} */
+ public void onBeforeDropNamespace(IcebergRestCatalogEvents.BeforeDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterDropNamespaceEvent} */
+ public void onAfterDropNamespace(IcebergRestCatalogEvents.AfterDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent} */
+ public void onBeforeUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent} */
+ public void onAfterUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent event) {}
+
+ // Iceberg REST Catalog Table Events
Review Comment:
Minor nits:
- We already have some table events declared above, maybe we should group them.
- Naming inconsistency: `onBeforeCreateTable` uses infinitive + noun, whereas `onAfterTableCommited` uses noun + past participle.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -777,8 +777,8 @@ public Response sendNotification(
securityContext,
prefix,
catalog -> {
- catalog.sendNotification(tableIdentifier, notificationRequest);
- return Response.status(Response.Status.NO_CONTENT).build();
+ boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);
Review Comment:
Unrelated change?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestConfigurationEventServiceDelegator.java:
##########
@@ -25,19 +25,28 @@
import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.rest.responses.ConfigResponse;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;
+import org.apache.polaris.service.events.PolarisEventListener;
@Decorator
@Priority(1000)
public class IcebergRestConfigurationEventServiceDelegator
implements IcebergRestConfigurationApiService {
Review Comment:
I noticed that you created a decorator for the config endpoint, but there is no decorator for the token endpoint (`IcebergRestOAuth2ApiService`). Is that on purpose?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -44,14 +96,24 @@
public class IcebergRestCatalogEventServiceDelegator implements IcebergRestCatalogApiService {
@Inject @Delegate IcebergCatalogAdapter delegate;
+ @Inject PolarisEventListener polarisEventListener;
@Override
public Response createNamespace(
String prefix,
CreateNamespaceRequest createNamespaceRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.createNamespace(prefix, createNamespaceRequest, realmContext, securityContext);
+ polarisEventListener.onBeforeCreateNamespace(
+ new BeforeCreateNamespaceEvent(prefix, createNamespaceRequest));
+ Response resp =
+ delegate.createNamespace(prefix, createNamespaceRequest, realmContext, securityContext);
+ CreateNamespaceResponse createNamespaceResponse =
+ resp.readEntity(CreateNamespaceResponse.class);
Review Comment:
`readEntity` is dangerous: it is supposed to read the response's input stream, and therefore should only be used on the client side, not the server side. Use `getEntity` instead.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #2480:
URL: https://github.com/apache/polaris/pull/2480#issuecomment-3304456517
Thanks @singhpk234 - makes sense. When we finish the E2E mechanism, we will surely either run the benchmark or mark as experimental.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2317632595
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -777,8 +777,8 @@ public Response sendNotification(
securityContext,
prefix,
catalog -> {
- catalog.sendNotification(tableIdentifier, notificationRequest);
- return Response.status(Response.Status.NO_CONTENT).build();
+ boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);
Review Comment:
No, at the moment we do not get whether the notification was sent successfully or not. So I've introduced this change so that we can surface that information. (There are code paths possible where the notificationSent can be false but the call still returns without error).
But to be fair, I'm not sure what notificationSent really helps with. So please let me know what you think.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java:
##########
@@ -55,4 +55,162 @@ public void onBeforeTaskAttempted(BeforeTaskAttemptedEvent event) {}
/** {@link AfterTaskAttemptedEvent} */
public void onAfterTaskAttempted(AfterTaskAttemptedEvent event) {}
+
+ // Iceberg REST Catalog Namespace Events
+ /** {@link IcebergRestCatalogEvents.BeforeCreateNamespaceEvent} */
+ public void onBeforeCreateNamespace(IcebergRestCatalogEvents.BeforeCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCreateNamespaceEvent} */
+ public void onAfterCreateNamespace(IcebergRestCatalogEvents.AfterCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeListNamespacesEvent} */
+ public void onBeforeListNamespaces(IcebergRestCatalogEvents.BeforeListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterListNamespacesEvent} */
+ public void onAfterListNamespaces(IcebergRestCatalogEvents.AfterListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent} */
+ public void onBeforeLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent} */
+ public void onAfterLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent} */
+ public void onBeforeCheckNamespaceExists(
+ IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent} */
+ public void onAfterCheckNamespaceExists(
+ IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeDropNamespaceEvent} */
+ public void onBeforeDropNamespace(IcebergRestCatalogEvents.BeforeDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterDropNamespaceEvent} */
+ public void onAfterDropNamespace(IcebergRestCatalogEvents.AfterDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent} */
+ public void onBeforeUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent} */
+ public void onAfterUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent event) {}
+
+ // Iceberg REST Catalog Table Events
Review Comment:
Good point, changed.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -62,26 +124,45 @@ public Response listNamespaces(
String parent,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.listNamespaces(
- prefix, pageToken, pageSize, parent, realmContext, securityContext);
+ polarisEventListener.onBeforeListNamespaces(new BeforeListNamespacesEvent(prefix, parent));
+ Response resp =
+ delegate.listNamespaces(prefix, pageToken, pageSize, parent, realmContext, securityContext);
+ polarisEventListener.onAfterListNamespaces(new AfterListNamespacesEvent(prefix, parent));
Review Comment:
That is the plan for the future, yes. Well, use try-finally rather than catch.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -44,14 +96,24 @@
public class IcebergRestCatalogEventServiceDelegator implements IcebergRestCatalogApiService {
@Inject @Delegate IcebergCatalogAdapter delegate;
+ @Inject PolarisEventListener polarisEventListener;
@Override
public Response createNamespace(
String prefix,
CreateNamespaceRequest createNamespaceRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.createNamespace(prefix, createNamespaceRequest, realmContext, securityContext);
+ polarisEventListener.onBeforeCreateNamespace(
+ new BeforeCreateNamespaceEvent(prefix, createNamespaceRequest));
+ Response resp =
+ delegate.createNamespace(prefix, createNamespaceRequest, realmContext, securityContext);
+ CreateNamespaceResponse createNamespaceResponse =
+ resp.readEntity(CreateNamespaceResponse.class);
Review Comment:
Thanks for pointing this out! Let me change this in the next revision.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestConfigurationEventServiceDelegator.java:
##########
@@ -25,19 +25,28 @@
import jakarta.inject.Inject;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.SecurityContext;
+import org.apache.iceberg.rest.responses.ConfigResponse;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService;
+import org.apache.polaris.service.events.IcebergRestCatalogEvents;
+import org.apache.polaris.service.events.PolarisEventListener;
@Decorator
@Priority(1000)
public class IcebergRestConfigurationEventServiceDelegator
implements IcebergRestConfigurationApiService {
Review Comment:
I purposefully left it alone as the `getToken` endpoint is deprecated for removal. Please let me know if you'd still like me to implement it regardless.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/IcebergRestCatalogEvents.java:
##########
@@ -0,0 +1,179 @@
+/*
+ * 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;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
+import org.apache.iceberg.rest.requests.CreateTableRequest;
+import org.apache.iceberg.rest.requests.CreateViewRequest;
+import org.apache.iceberg.rest.requests.RegisterTableRequest;
+import org.apache.iceberg.rest.requests.RenameTableRequest;
+import org.apache.iceberg.rest.requests.UpdateNamespacePropertiesRequest;
+import org.apache.iceberg.rest.responses.ConfigResponse;
+import org.apache.iceberg.rest.responses.LoadTableResponse;
+import org.apache.iceberg.rest.responses.LoadViewResponse;
+import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse;
+import org.apache.polaris.service.types.CommitTableRequest;
+import org.apache.polaris.service.types.CommitViewRequest;
+import org.apache.polaris.service.types.NotificationRequest;
+
+/**
+ * Event records for Iceberg REST Catalog operations. Each operation has corresponding "Before" and
+ * "After" event records.
+ */
+public class IcebergRestCatalogEvents {
Review Comment:
Agreed, included in the next revision!
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2318196871
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -307,7 +519,18 @@ public Response sendNotification(
NotificationRequest notificationRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.sendNotification(
- prefix, namespace, table, notificationRequest, realmContext, securityContext);
+ String catalogName = getCatalogFromPrefix(prefix, realmContext);
+ polarisEventListener.onBeforeSendNotification(
+ new BeforeSendNotificationEvent(catalogName, namespace, table, notificationRequest));
+ Response resp =
+ delegate.sendNotification(
+ prefix, namespace, table, notificationRequest, realmContext, securityContext);
+ polarisEventListener.onAfterSendNotification(
+ new AfterSendNotificationEvent(catalogName, namespace, table, (boolean) resp.getEntity()));
+ return resp;
+ }
+
+ private String getCatalogFromPrefix(String prefix, RealmContext realmContext) {
+ return prefixParser.prefixToCatalogName(realmContext, prefix);
Review Comment:
nit: inline.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java:
##########
@@ -55,4 +55,162 @@ public void onBeforeTaskAttempted(BeforeTaskAttemptedEvent event) {}
/** {@link AfterTaskAttemptedEvent} */
public void onAfterTaskAttempted(AfterTaskAttemptedEvent event) {}
+
+ // Iceberg REST Catalog Namespace Events
+ /** {@link IcebergRestCatalogEvents.BeforeCreateNamespaceEvent} */
+ public void onBeforeCreateNamespace(IcebergRestCatalogEvents.BeforeCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCreateNamespaceEvent} */
+ public void onAfterCreateNamespace(IcebergRestCatalogEvents.AfterCreateNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeListNamespacesEvent} */
+ public void onBeforeListNamespaces(IcebergRestCatalogEvents.BeforeListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterListNamespacesEvent} */
+ public void onAfterListNamespaces(IcebergRestCatalogEvents.AfterListNamespacesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent} */
+ public void onBeforeLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.BeforeLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent} */
+ public void onAfterLoadNamespaceMetadata(
+ IcebergRestCatalogEvents.AfterLoadNamespaceMetadataEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent} */
+ public void onBeforeCheckNamespaceExists(
+ IcebergRestCatalogEvents.BeforeCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent} */
+ public void onAfterCheckNamespaceExists(
+ IcebergRestCatalogEvents.AfterCheckNamespaceExistsEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeDropNamespaceEvent} */
+ public void onBeforeDropNamespace(IcebergRestCatalogEvents.BeforeDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterDropNamespaceEvent} */
+ public void onAfterDropNamespace(IcebergRestCatalogEvents.AfterDropNamespaceEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent} */
+ public void onBeforeUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.BeforeUpdateNamespacePropertiesEvent event) {}
+
+ /** {@link IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent} */
+ public void onAfterUpdateNamespaceProperties(
+ IcebergRestCatalogEvents.AfterUpdateNamespacePropertiesEvent event) {}
+
+ // Iceberg REST Catalog Table Events
Review Comment:
Thanks, that's better! But we still have inconsistencies, see my comment:
https://github.com/apache/polaris/pull/2479#discussion_r2318169836
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -777,8 +777,8 @@ public Response sendNotification(
securityContext,
prefix,
catalog -> {
- catalog.sendNotification(tableIdentifier, notificationRequest);
- return Response.status(Response.Status.NO_CONTENT).build();
+ boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);
Review Comment:
In this case this requires a modification of the OpenAPI spec. See my related comment:
https://github.com/apache/polaris/pull/2482/files#r2318079257
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2318253912
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java:
##########
@@ -777,8 +777,8 @@ public Response sendNotification(
securityContext,
prefix,
catalog -> {
- catalog.sendNotification(tableIdentifier, notificationRequest);
- return Response.status(Response.Status.NO_CONTENT).build();
+ boolean notificationSent = catalog.sendNotification(tableIdentifier, notificationRequest);
Review Comment:
Also: `status(Response.Status.NO_CONTENT).entity(notificationSent)` is contradictory: the response for a 204 status should not contain any body:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/204
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2326120030
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -103,8 +202,29 @@ public Response createTable(
String accessDelegationMode,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.createTable(
- prefix, namespace, createTableRequest, accessDelegationMode, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ if (!createTableRequest.stageCreate()) {
Review Comment:
why don't we emit anything for StageCreate ?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -273,10 +477,21 @@ public Response replaceView(
CommitViewRequest commitViewRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.replaceView(
- prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeReplaceView(
+ new BeforeReplaceViewEvent(catalogName, namespace, view, commitViewRequest));
+ Response resp =
+ delegate.replaceView(
+ prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ polarisEventListener.onAfterReplaceView(
+ new AfterReplaceViewEvent(
+ catalogName, namespace, view, (LoadViewResponse) resp.getEntity()));
+ return resp;
}
+ /**
+ * Table Committed Events are already instrumented at a more granular level than the API itself.
+ */
Review Comment:
how and why , are it we tracking it per table ?? this seems like it doesn't follow the pattern
-- BeforEvent
-- do Something
-- After Event
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -212,8 +384,12 @@ public Response listViews(
Integer pageSize,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.listViews(
- prefix, namespace, pageToken, pageSize, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeListViews(new BeforeListViewsEvent(catalogName, namespace));
Review Comment:
are the nested namespace encoded by '%1F' in the final even ?
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2331371789
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -103,8 +202,29 @@ public Response createTable(
String accessDelegationMode,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.createTable(
- prefix, namespace, createTableRequest, accessDelegationMode, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ if (!createTableRequest.stageCreate()) {
Review Comment:
My understanding is that when a table is stageCreated, then no actual table is made - and therefore there is no new (meta)data created. In that case, I don't think it makes sense to emit a `AfterCreateTableEvent` if a table was not truly created.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -212,8 +384,12 @@ public Response listViews(
Integer pageSize,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.listViews(
- prefix, namespace, pageToken, pageSize, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeListViews(new BeforeListViewsEvent(catalogName, namespace));
Review Comment:
This is a good callout - we should switch this to use `Namespace` objects instead. Doing in the next revision.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -273,10 +477,21 @@ public Response replaceView(
CommitViewRequest commitViewRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.replaceView(
- prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeReplaceView(
+ new BeforeReplaceViewEvent(catalogName, namespace, view, commitViewRequest));
+ Response resp =
+ delegate.replaceView(
+ prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ polarisEventListener.onAfterReplaceView(
+ new AfterReplaceViewEvent(
+ catalogName, namespace, view, (LoadViewResponse) resp.getEntity()));
+ return resp;
}
+ /**
+ * Table Committed Events are already instrumented at a more granular level than the API itself.
+ */
Review Comment:
The TableCommittedEvent was previously merged and there are no changes to it as part of this PR. The idea is that all table commits that happened as part of the same Polaris API call (through the `commitTransaction` API) will already be tied together through the same request ID and so a new Event for this API is not explicitly required.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2331558418
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -273,10 +477,21 @@ public Response replaceView(
CommitViewRequest commitViewRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.replaceView(
- prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeReplaceView(
+ new BeforeReplaceViewEvent(catalogName, namespace, view, commitViewRequest));
+ Response resp =
+ delegate.replaceView(
+ prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ polarisEventListener.onAfterReplaceView(
+ new AfterReplaceViewEvent(
+ catalogName, namespace, view, (LoadViewResponse) resp.getEntity()));
+ return resp;
}
+ /**
+ * Table Committed Events are already instrumented at a more granular level than the API itself.
+ */
Review Comment:
On second thought, you are correct - the Table Committed events are triggered even for non-transactional events. Let me add this.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2331431006
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -273,10 +477,21 @@ public Response replaceView(
CommitViewRequest commitViewRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.replaceView(
- prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeReplaceView(
+ new BeforeReplaceViewEvent(catalogName, namespace, view, commitViewRequest));
+ Response resp =
+ delegate.replaceView(
+ prefix, namespace, view, commitViewRequest, realmContext, securityContext);
+ polarisEventListener.onAfterReplaceView(
+ new AfterReplaceViewEvent(
+ catalogName, namespace, view, (LoadViewResponse) resp.getEntity()));
+ return resp;
}
+ /**
+ * Table Committed Events are already instrumented at a more granular level than the API itself.
+ */
Review Comment:
what is the on before and on after for this, would looks though ?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -103,8 +202,29 @@ public Response createTable(
String accessDelegationMode,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.createTable(
- prefix, namespace, createTableRequest, accessDelegationMode, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ if (!createTableRequest.stageCreate()) {
Review Comment:
But we are not emitting a onBeforeCreateTable too ? never the less what happens when i stage create and then do `/commit` ?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -262,7 +480,13 @@ public Response renameView(
RenameTableRequest renameTableRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.renameView(prefix, renameTableRequest, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeRenameView(
+ new BeforeRenameViewEvent(catalogName, renameTableRequest));
Review Comment:
no namespace here ? wouldn't it be nice to know namespace before after ?
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #2480:
URL: https://github.com/apache/polaris/pull/2480#discussion_r2331569609
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -262,7 +480,13 @@ public Response renameView(
RenameTableRequest renameTableRequest,
RealmContext realmContext,
SecurityContext securityContext) {
- return delegate.renameView(prefix, renameTableRequest, realmContext, securityContext);
+ String catalogName = prefixParser.prefixToCatalogName(realmContext, prefix);
+ polarisEventListener.onBeforeRenameView(
+ new BeforeRenameViewEvent(catalogName, renameTableRequest));
Review Comment:
`renameTableRequest` has the before and after table identifiers, where an event listener can access that the namespace information.
--
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] Add Events for Iceberg REST APIs [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 merged PR #2480:
URL: https://github.com/apache/polaris/pull/2480
--
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