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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
