adnanhemani commented on code in PR #1904: URL: https://github.com/apache/polaris/pull/1904#discussion_r2153366463
########## service/common/src/main/java/org/apache/polaris/service/events/AfterAddGrantToCatalogRoleEvent.java: ########## @@ -0,0 +1,83 @@ +/* + * 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 org.apache.polaris.core.admin.model.AddGrantRequest; +import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal; +import org.apache.polaris.core.entity.PolarisPrivilege; + +/** Event fired after a grant is added to a catalog role in Polaris. */ +public class AfterAddGrantToCatalogRoleEvent implements PolarisEvent { Review Comment: Same response as: https://github.com/apache/polaris/pull/1844#discussion_r2152337757 ########## service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java: ########## @@ -142,12 +211,24 @@ private PolarisAdminService newAdminService( @Override public Response createCatalog( CreateCatalogRequest request, RealmContext realmContext, SecurityContext securityContext) { + String requestId = PolarisEvent.createRequestId(); Review Comment: Re: execution flow Personally, I'm okay if we do not give a successful execution back to the user if the event listeners throw an error. For the use case in which customers actually add perhaps additional authorization or validations to a call through an event listener, this is exactly what the customer would expect. This is how event listeners are currently instrumented today as well - and I don't see a solid reasoning to change this paradigm right now. (Event Listeners and their configuration are still the responsibility of the Polaris admin, as this is basically like a custom plugin; additionally, the sample events-to-persistence implementations in #1844 take this into account and purposefully will not throw errors during the execution of the event listener in-line code). ---- Re: decorator I believe the pattern you are referring to includes defining a @Decorator class which would also implement `PolarisCatalogsApiService`/`PolarisPrincipalsApiService`/`PolarisPrincipalRolesApiService` and then use the `PolarisServiceImpl` class as a delegator? If this is not correct, please do help provide some sample/hints that would make it clearer what you mean. But going back - in the case of the decorator+delegator, sure this may make this particular file cleaner - but the same amount of code will exist as we will just be moving the event listener hooks to a different file and then calling `PolarisServiceImpl` from there. If you think that slight amount of cleanliness outweighs the redirection, I'm happy to take this as an action item. Wanted to confirm what you mean before going ahead with an implementation on 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
