yuqi1129 commented on code in PR #4109: URL: https://github.com/apache/gravitino/pull/4109#discussion_r1670461569
########## core/src/main/java/com/datastrato/gravitino/SupportsExtraOperations.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 com.datastrato.gravitino; + +import com.datastrato.gravitino.exceptions.NoSuchEntityException; +import com.datastrato.gravitino.meta.TagEntity; +import java.io.IOException; + +/** + * An interface to support extra entity store operations, this interface should be mixed with {@link + * EntityStore} to provide extra operations. + * + * <p>Any operations that can be done by the entity store should be added here. + */ +public interface SupportsExtraOperations { + + /** + * List all the metadata objects that are associated with the given tag. + * + * @param tagIdent The identifier of the tag. + * @return The list of metadata objects associated with the given tag. + */ + MetadataObject[] listAssociatedMetadataObjectsForTag(NameIdentifier tagIdent) throws IOException; Review Comment: `MetadataObject` seems to be the upper concept of `Entity`, I think whether we need to create a new interface that handles business logic above the `EntityStore` specially and make the `EntityStore` handle `Entity`only. ########## api/src/main/java/com/datastrato/gravitino/exceptions/TagAlreadyAssociatedException.java: ########## @@ -0,0 +1,49 @@ +/* + * 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 com.datastrato.gravitino.exceptions; + +import com.google.errorprone.annotations.FormatMethod; + +/** Exception thrown when a tag with specified name already associated to a metadata object. */ +public class TagAlreadyAssociatedException extends AlreadyExistsException { Review Comment: In my opinion, `AlreadyExistsException` means an entity already exists in our system, so, I'm wondering whether it's reasonable to make `TagAlreadyAssociatedException` as the sub-class of `AlreadyExistsException`. ########## core/src/main/java/com/datastrato/gravitino/EntityStore.java: ########## @@ -181,4 +181,14 @@ default boolean delete(NameIdentifier ident, EntityType entityType) throws IOExc */ <R, E extends Exception> R executeInTransaction(Executable<R, E> executable) throws E, IOException; + + /** + * Get the extra operations that are supported by the entity store. + * + * @return the extra operations object that are supported by the entity store + * @throws UnsupportedOperationException if the extra operations are not supported + */ + default SupportsExtraOperations extraOperations() { Review Comment: If we plan to use the way to handle some specific operations, I believe the word `extra` is too vague. ########## core/src/main/java/com/datastrato/gravitino/SupportsExtraOperations.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 com.datastrato.gravitino; + +import com.datastrato.gravitino.exceptions.NoSuchEntityException; +import com.datastrato.gravitino.meta.TagEntity; +import java.io.IOException; + +/** + * An interface to support extra entity store operations, this interface should be mixed with {@link + * EntityStore} to provide extra operations. + * + * <p>Any operations that can be done by the entity store should be added here. + */ +public interface SupportsExtraOperations { + + /** + * List all the metadata objects that are associated with the given tag. + * + * @param tagIdent The identifier of the tag. + * @return The list of metadata objects associated with the given tag. + */ + MetadataObject[] listAssociatedMetadataObjectsForTag(NameIdentifier tagIdent) throws IOException; Review Comment: Though adding `SupportsExtraOperations extraOperations()` will not damange the inteface `EntityStore` generality, it still makes the interface hard to extend. ########## core/src/main/java/com/datastrato/gravitino/utils/MetadataObjectUtil.java: ########## @@ -0,0 +1,89 @@ +/* + * 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 com.datastrato.gravitino.utils; + +import com.datastrato.gravitino.Entity; +import com.datastrato.gravitino.MetadataObject; +import com.datastrato.gravitino.NameIdentifier; +import com.google.common.base.Joiner; + +public class MetadataObjectUtil { + + private static final Joiner DOT = Joiner.on("."); + + private MetadataObjectUtil() {} + + /** + * Map the given {@link MetadataObject}'s type to the corresponding {@link Entity.EntityType}. + * + * @param metadataObject The metadata object + * @return The entity type + * @throws IllegalArgumentException if the metadata object type is unknown + */ + public static Entity.EntityType toEntityType(MetadataObject metadataObject) { Review Comment: Using a binary map to keep the mapping between `MetadataObject.Type` and `MetadataObject` will be more sufficient. ########## core/src/main/java/com/datastrato/gravitino/utils/NameIdentifierUtil.java: ########## @@ -193,4 +198,54 @@ public static void check(boolean expression, @FormatString String message, Objec throw new IllegalNamespaceException(message, args); } } + + /** + * Convert the given {@link NameIdentifier} and {@link Entity.EntityType} to {@link + * MetadataObject}. + * + * @param ident The identifier + * @param entityType The entity type + * @return The converted {@link MetadataObject} + */ + public static MetadataObject toMetadataObject( + NameIdentifier ident, Entity.EntityType entityType) { + Preconditions.checkArgument( + ident != null && entityType != null, "The identifier and entity type must not be null"); + + Joiner dot = Joiner.on("."); + + switch (entityType) { + case METALAKE: + checkMetalake(ident); + return MetadataObjects.of(null, ident.name(), MetadataObject.Type.METALAKE); + + case CATALOG: + checkCatalog(ident); + return MetadataObjects.of(null, ident.name(), MetadataObject.Type.CATALOG); Review Comment: Why is the parent of a catalog metadata object null? -- 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]
