Copilot commented on code in PR #9475: URL: https://github.com/apache/gravitino/pull/9475#discussion_r2613901478
########## core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.gravitino.catalog; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.utils.NameIdentifierUtil; + +public class OperationDispatcherInterceptor implements InvocationHandler { + private final Object dispatcher; + private final CatalogManager catalogManager; + private final EntityStore store; + + public OperationDispatcherInterceptor( + Object dispatcher, CatalogManager catalogManager, EntityStore store) { + this.dispatcher = dispatcher; + this.catalogManager = catalogManager; + this.store = store; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (args != null && args.length > 0) { + NameIdentifier catalogIdent = null; + if (args[0] instanceof NameIdentifier ident) { + catalogIdent = NameIdentifierUtil.getCatalogIdentifier(ident); + } else if (args[0] instanceof Namespace ns) { + if (ns.length() >= 2) { + catalogIdent = NameIdentifier.of(ns.level(0), ns.level(1)); + } + } + + if (catalogIdent != null) { + final NameIdentifier finalCatalogIdent = catalogIdent; + // This will break logic into two compared to that in the original code. Review Comment: The comment "This will break logic into two compared to that in the original code" is unclear and potentially misleading. If this refers to the separation of the catalog check from the actual operation, it should be explained more clearly. If this is a known issue or trade-off, it should be documented more thoroughly with information about why this approach was chosen despite the concern. ```suggestion // Note: In this implementation, the catalog-in-use check is performed separately // under a tree lock before invoking the main operation. In the original code, // this check may have been performed as part of a single, monolithic operation. // This separation ensures that the catalog's state is validated under the appropriate // lock, improving thread safety and consistency. However, it introduces a trade-off: // the check and the main operation are not atomic with respect to each other, so there // is a small window where the catalog's state could change between the check and the operation. // This approach was chosen to avoid holding locks during potentially long-running operations, // balancing safety and performance. ``` ########## core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.gravitino.catalog; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.utils.NameIdentifierUtil; + +public class OperationDispatcherInterceptor implements InvocationHandler { + private final Object dispatcher; + private final CatalogManager catalogManager; + private final EntityStore store; + + public OperationDispatcherInterceptor( + Object dispatcher, CatalogManager catalogManager, EntityStore store) { + this.dispatcher = dispatcher; + this.catalogManager = catalogManager; + this.store = store; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (args != null && args.length > 0) { + NameIdentifier catalogIdent = null; + if (args[0] instanceof NameIdentifier ident) { + catalogIdent = NameIdentifierUtil.getCatalogIdentifier(ident); + } else if (args[0] instanceof Namespace ns) { + if (ns.length() >= 2) { + catalogIdent = NameIdentifier.of(ns.level(0), ns.level(1)); + } + } + + if (catalogIdent != null) { + final NameIdentifier finalCatalogIdent = catalogIdent; + // This will break logic into two compared to that in the original code. + TreeLockUtils.doWithTreeLock( + catalogIdent, + LockType.READ, + () -> { + catalogManager.checkCatalogInUse(store, finalCatalogIdent); + return null; + }); + } + } + return method.invoke(dispatcher, args); + } +} Review Comment: The new OperationDispatcherInterceptor class lacks dedicated unit tests. While existing integration tests may cover the behavior indirectly, a new class with this much responsibility should have explicit unit tests to verify: 1) catalog identifier extraction from both NameIdentifier and Namespace parameters, 2) correct handling when args are null or empty, 3) correct handling when catalog identifier cannot be extracted (e.g., namespace with less than 2 levels), 4) exception propagation from checkCatalogInUse, and 5) that the underlying dispatcher method is called after the check. ########## core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.gravitino.catalog; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.utils.NameIdentifierUtil; + +public class OperationDispatcherInterceptor implements InvocationHandler { + private final Object dispatcher; + private final CatalogManager catalogManager; + private final EntityStore store; + + public OperationDispatcherInterceptor( + Object dispatcher, CatalogManager catalogManager, EntityStore store) { + this.dispatcher = dispatcher; + this.catalogManager = catalogManager; + this.store = store; + } Review Comment: The constructor is missing Javadoc documentation. According to Apache Gravitino guidelines, public constructors should have documentation including @param tags for all parameters. ########## core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.gravitino.catalog; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.utils.NameIdentifierUtil; + +public class OperationDispatcherInterceptor implements InvocationHandler { + private final Object dispatcher; Review Comment: The class is missing Javadoc documentation. According to Apache Gravitino guidelines, public classes should have complete documentation explaining their purpose, how they work, and any important behavioral characteristics. This is especially important for a class that uses the dynamic proxy pattern, as it affects all dispatcher operations. ```suggestion /** * An {@link InvocationHandler} implementation that intercepts method calls on dispatcher objects * in the Gravitino catalog system. This class is used as part of the dynamic proxy pattern to * wrap dispatcher instances, enabling pre-processing logic such as catalog existence checks and * tree-based locking before delegating the actual method invocation to the underlying dispatcher. * <p> * For each intercepted method call, if the first argument is a {@link NameIdentifier} or * {@link Namespace}, the interceptor extracts the catalog identifier and acquires a read lock * on the catalog using {@link TreeLockUtils}. It then checks if the catalog is in use via the * {@link CatalogManager}. Only after these checks and locks does it invoke the original method * on the dispatcher. * <p> * This mechanism ensures that all dispatcher operations are performed safely and consistently * with respect to catalog state and concurrency requirements. */ public class OperationDispatcherInterceptor implements InvocationHandler { ``` ########## core/src/main/java/org/apache/gravitino/catalog/OperationDispatcherInterceptor.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.gravitino.catalog; + +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.lock.LockType; +import org.apache.gravitino.lock.TreeLockUtils; +import org.apache.gravitino.utils.NameIdentifierUtil; + +public class OperationDispatcherInterceptor implements InvocationHandler { + private final Object dispatcher; + private final CatalogManager catalogManager; + private final EntityStore store; + + public OperationDispatcherInterceptor( + Object dispatcher, CatalogManager catalogManager, EntityStore store) { + this.dispatcher = dispatcher; + this.catalogManager = catalogManager; + this.store = store; + } + + @Override + public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { + if (args != null && args.length > 0) { + NameIdentifier catalogIdent = null; + if (args[0] instanceof NameIdentifier ident) { + catalogIdent = NameIdentifierUtil.getCatalogIdentifier(ident); + } else if (args[0] instanceof Namespace ns) { + if (ns.length() >= 2) { + catalogIdent = NameIdentifier.of(ns.level(0), ns.level(1)); + } + } + + if (catalogIdent != null) { + final NameIdentifier finalCatalogIdent = catalogIdent; + // This will break logic into two compared to that in the original code. + TreeLockUtils.doWithTreeLock( + catalogIdent, + LockType.READ, + () -> { + catalogManager.checkCatalogInUse(store, finalCatalogIdent); + return null; + }); + } + } + return method.invoke(dispatcher, args); Review Comment: The catalog in-use check is performed under a lock that is released before the actual dispatcher method is invoked. This creates a time window where the catalog could be disabled between the check at line 61 and the actual operation at line 66. While the comment acknowledges "this will break logic into two," it's unclear if this race condition is acceptable. The original code in doWithCatalog also had this issue, but it's worth documenting why this design trade-off is acceptable for the performance improvement, or if additional safeguards are needed. Consider adding a detailed comment explaining the concurrency implications and why the risk is acceptable. -- 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]
