snazy commented on code in PR #922: URL: https://github.com/apache/polaris/pull/922#discussion_r2000620772
########## service/common/src/main/java/org/apache/polaris/service/events/DefaultPolarisEventListener.java: ########## @@ -0,0 +1,27 @@ +/* + * 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 io.smallrye.common.annotation.Identifier; +import jakarta.enterprise.context.ApplicationScoped; + +/** Event listener that does nothing. */ +@ApplicationScoped +@Identifier("default") +public class DefaultPolarisEventListener extends PolarisEventListener {} Review Comment: Should be called `Noop...`, not "default", because "default" implies that it does something. ########## service/common/src/main/java/org/apache/polaris/service/events/TestPolarisEventListener.java: ########## @@ -0,0 +1,91 @@ +/* + * 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 com.google.common.collect.Streams; +import io.smallrye.common.annotation.Identifier; +import jakarta.enterprise.context.ApplicationScoped; +import java.util.ArrayList; +import java.util.List; + +/** Event listener that stores all emitted events forever. Not recommended for use in production. */ +@ApplicationScoped +@Identifier("test") Review Comment: This is test-only code and shouldn't be reachable in production code at all. ########## service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java: ########## @@ -127,6 +128,7 @@ public class IcebergCatalogAdapter private final PolarisMetaStoreManager metaStoreManager; private final PolarisAuthorizer polarisAuthorizer; private final IcebergCatalogPrefixParser prefixParser; + private final PolarisEventListener polarisEventListener; Review Comment: Why having an unused field? ########## service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java: ########## @@ -1229,11 +1243,14 @@ public void doRefresh() { Set.of(PolarisStorageActions.READ)); return TableMetadataParser.read(fileIO, metadataLocation); }); + polarisEventListener.onAfterTableRefreshed(new AfterTableRefreshedEvent(tableIdentifier)); Review Comment: Don't failures deserve to be emitted? ########## service/common/src/main/java/org/apache/polaris/service/events/BeforeRequestRateLimitedEvent.java: ########## @@ -0,0 +1,28 @@ +/* + * 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; + +/** + * Emitted before the RateLimiterFilter rejects a request due to exceeding the rate limit. + * + * @param method The request's HTTP method + * @param absolutePath The request's absolute path + */ +public record BeforeRequestRateLimitedEvent(String method, String absolutePath) Review Comment: What's the use case of this one and how would an implementation behave in case of a DoS attack attempt? ########## service/common/src/main/java/org/apache/polaris/service/events/AfterTableCommitedEvent.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.iceberg.TableMetadata; + +/** + * Emitted after Polaris performs a commit to a table. This is not emitted if there's an exception + * while committing. + * + * @param base The old metadata. + * @param metadata The new metadata. + */ +public record AfterTableCommitedEvent(TableMetadata base, TableMetadata metadata) Review Comment: What's the identifier of the table?? ########## service/common/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java: ########## @@ -0,0 +1,58 @@ +/* + * 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; + +/** + * Represents an event listener that can respond to notable moments during Polaris's execution. + * Event details are documented under the event objects themselves. + */ +public abstract class PolarisEventListener { Review Comment: What's the plan/approach for this type when new events are added? For example: generic tables - are those handled via `on*Table*` or do those get separate events? Style: this is rather an `interface`. ########## service/common/src/main/java/org/apache/polaris/service/events/AfterTaskAttemptedEvent.java: ########## @@ -0,0 +1,34 @@ +/* + * 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.context.CallContext; + +/** + * Emitted after an attempt of an async task, such as manifest file cleanup, finishes. + * + * @param taskEntityId The ID of the TaskEntity. + * @param callContext The CallContext the task is being executed under. + * @param attempt The attempt number. Each retry of the task will have its own attempt number. The + * initial (non-retried) attempt starts counting from 1. + * @param success Whether or not the attempt succeeded. + */ +public record AfterTaskAttemptedEvent( + long taskEntityId, CallContext callContext, int attempt, boolean success) Review Comment: `taskEntityId` is a persistence internal property, `attempt` is actually wrong - see how `loadTasks` is implemented. I'm also not sure this event makes sense, given that task handling is incomplete. ########## service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java: ########## @@ -123,6 +124,7 @@ public class PolarisCatalogHandlerWrapper implements AutoCloseable { private final SecurityContext securityContext; private final PolarisAuthorizer authorizer; private final CallContextCatalogFactory catalogFactory; + private final PolarisEventListener polarisEventListener; Review Comment: Why having an unused field? ########## service/common/src/main/java/org/apache/polaris/service/events/AfterViewCommitedEvent.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.iceberg.view.ViewMetadata; + +/** + * Emitted after Polaris performs a commit to a view. This is not emitted if there's an exception + * while committing. + * + * @param base The old metadata. + * @param metadata The new metadata. + */ +public record AfterViewCommitedEvent(ViewMetadata base, ViewMetadata metadata) Review Comment: What's the identifier of the view?? ########## service/common/src/main/java/org/apache/polaris/service/events/AfterTaskAttemptedEvent.java: ########## @@ -0,0 +1,34 @@ +/* + * 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.context.CallContext; + +/** + * Emitted after an attempt of an async task, such as manifest file cleanup, finishes. + * + * @param taskEntityId The ID of the TaskEntity. + * @param callContext The CallContext the task is being executed under. + * @param attempt The attempt number. Each retry of the task will have its own attempt number. The + * initial (non-retried) attempt starts counting from 1. + * @param success Whether or not the attempt succeeded. + */ +public record AfterTaskAttemptedEvent( + long taskEntityId, CallContext callContext, int attempt, boolean success) Review Comment: Can you elaborate on the use case of this one? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
