yuqi1129 commented on code in PR #13881: URL: https://github.com/apache/gravitino/pull/13881#discussion_r4389462869
########## api/src/main/java/org/apache/gravitino/domain/SupportsDomains.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.domain; + +import org.apache.gravitino.annotation.Evolving; + +/** + * Interface for listing the domains of a leaf metadata object, such as a table, view or fileset. + * Catalogs and schemas do not support it, since they are never members of a domain; the domain a + * catalog or schema passes down to the objects under it is visible from its tags. + * + * <p>The domains listed are all the domains visible to the caller whose scope contains the object: + * the domains whose backing tag the object's effective tags match, and the parents of those that + * are sub-domains visible to the caller. The effective tags are the object's own tags plus those + * inherited from its schema and catalog; for the same tag, the assignment nearest to the object + * wins. Each domain comes with its {@link Domain#relations() relations} to the object; an object + * tagged only with the backing tag of sub-domain "Trading" lists Trading as {@link + * DomainRelation.Type#DIRECT} and its parent "Finance" as {@link DomainRelation.Type#VIA_SUBDOMAIN} + * Trading. + * + * <p>To add an object to a domain or remove it, associate the domain's backing tag with the object Review Comment: [Nit] Please qualify the removal guidance for inherited membership. Removing a direct tag assignment from a leaf object does not necessarily remove it from the domain: it may still inherit the backing tag from its schema/catalog, and removing an overriding assignment can expose an inherited assignment again. The design document already explains this distinction. It would help API callers if this paragraph explicitly said that SupportsTags changes direct assignments and membership is then recomputed from effective tags, so an inherited membership cannot simply be removed at the leaf. ########## api/src/main/java/org/apache/gravitino/domain/Domain.java: ########## @@ -0,0 +1,278 @@ +/* + * 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.domain; + +import java.util.Map; +import java.util.Optional; +import javax.annotation.Nullable; +import org.apache.gravitino.Auditable; +import org.apache.gravitino.annotation.Evolving; +import org.apache.gravitino.exceptions.BackingTagAlreadyBoundException; +import org.apache.gravitino.exceptions.DomainAlreadyExistsException; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.NoSuchDomainException; +import org.apache.gravitino.exceptions.NoSuchTagException; +import org.apache.gravitino.exceptions.TagAlreadyExistsException; +import org.apache.gravitino.tag.TagValue; + +/** + * A domain groups metadata objects from any catalog under a business name, such as "Finance". A + * leaf metadata object, of a type in {@link DomainMember#SUPPORTED_TYPES}, is a member of a domain + * when it carries the domain's backing tag, directly or inherited from its catalog or schema; + * catalogs and schemas are never members themselves. For the same tag, the assignment nearest to + * the object wins: a table that carries {@code dept=hr} under a schema that carries {@code + * dept=finance} is a member of the domain backed by {@code dept=hr} only. + * + * <p>A domain is either top-level or a sub-domain of exactly one top-level domain. Privileges on a + * domain, and its ownership, also apply to its sub-domains: the metalake, the current parent and + * the domain itself are checked together. An owner of any of them passes; otherwise a privilege + * holds when it is allowed on at least one of them and denied on none, so a deny on a sub-domain is + * not overridden by an allow on its parent. + * + * <p>A new domain is in {@link Status#DRAFT} status and, until it is published, is returned only to + * the users who can manage it. + * + * <p>A {@code Domain} is a snapshot. The operations on it that change the domain itself, such as + * {@link #publish()} and {@link #attachToParent(String)}, return a new snapshot and leave this one + * unchanged. They apply to the current state of the domain, not to the state this snapshot shows, + * and the last write wins. They address the domain by the name this snapshot shows: if the domain + * is renamed and a new domain takes its old name, they act on the new domain. + */ +@Evolving +public interface Domain extends Auditable { + + /** The status of a domain. */ + enum Status { + /** + * The domain is returned only to the users who can manage it, including through its parent or + * the metalake. + */ + DRAFT, + /** The domain is returned to all users who can use it. */ + PUBLISHED + } + + /** + * Returns the name of the domain. + * + * @return The name of the domain. + */ + String name(); + + /** + * Returns the comment of the domain. + * + * @return The comment of the domain, or null if it is not set. + */ + @Nullable + String comment(); + + /** + * Returns the properties of the domain. + * + * @return The properties of the domain. + */ + Map<String, String> properties(); + + /** + * Returns the status of the domain. + * + * @return The status of the domain. + */ + Status status(); + + /** + * Returns the name of the parent domain. The name is returned even when the parent is not visible + * to the caller, for example a draft parent; getting that parent still fails with {@link + * NoSuchDomainException}. + * + * @return The name of the parent domain, or empty if the domain is top-level. + */ + Optional<String> parent(); + + /** + * Returns the backing tag of the domain. The metadata objects whose effective assignment of this + * tag matches it are the members of the domain. A backing tag without a value matches every + * assignment of the tag, with or without values. A backing tag with a value matches the + * assignments whose values contain it, compared exactly and case-sensitively. + * + * <p>The domain refers to the tag itself, not to its name: a renamed tag keeps backing the + * domain, and a new tag with the name of a deleted one does not. + * + * @return The backing tag of the domain, or empty if the backing tag has been removed or deleted + * and no new one has been set. + */ + Optional<TagValue> backingTag(); + + /** + * Returns how the metadata object relates to this domain. It is set only on the domains returned + * by {@link SupportsDomains#listDomainsInfo()}, and lists every relation that applies, with no + * precedence among them. + * + * @return The relations of the metadata object to this domain, or an empty array if the domain is + * not listed for a metadata object. + */ + DomainRelation[] relations(); + + /** + * Publishes the domain, making it visible to all users who can use it. A sub-domain can be + * published only when its parent is published. Publishing a published domain has no effect. + * + * @return The published domain. + * @throws NoSuchDomainException If the domain does not exist or is not visible to the caller. + * @throws IllegalArgumentException If the domain is a sub-domain and its parent is not published. + */ + Domain publish() throws NoSuchDomainException, IllegalArgumentException; + + /** + * Unpublishes the domain, making it visible only to the users who can manage it. A domain can be + * unpublished only when it has no published sub-domains. Unpublishing a draft domain has no + * effect. + * + * @return The unpublished domain. + * @throws NoSuchDomainException If the domain does not exist or is not visible to the caller. + * @throws IllegalArgumentException If the domain has published sub-domains. + */ + Domain unpublish() throws NoSuchDomainException, IllegalArgumentException; + + /** + * Attaches this domain as a sub-domain of the given parent. If this domain is a sub-domain of + * another domain, it is moved under the given parent. Attaching to the current parent has no + * effect. The backing tag is not renamed. + * + * <p>Since privileges on a parent, and its ownership, apply to its sub-domains, attaching or + * moving changes who can see and manage this domain without any grant changing: those of the old + * parent no longer apply to this domain, and those of the new parent do. A deny on this domain + * still applies. + * + * @param parent The name of the parent domain. + * @return The attached domain. + * @throws NoSuchDomainException If this domain or the parent does not exist or is not visible to + * the caller. + * @throws IllegalArgumentException If this domain cannot be attached to the parent, for example + * because the parent is this domain or a sub-domain, this domain has sub-domains, or this + * domain is published and the parent is not. + */ + Domain attachToParent(String parent) throws NoSuchDomainException, IllegalArgumentException; + + /** + * Detaches this domain from its current parent, making it a top-level domain, whatever parent + * this snapshot shows. Detaching a top-level domain has no effect. The backing tag is not + * renamed. + * + * <p>Since privileges on a parent, and its ownership, apply to its sub-domains, detaching changes + * who can see and manage this domain without any grant changing: those of the former parent no + * longer apply to this domain. + * + * @return The detached domain. + * @throws NoSuchDomainException If this domain does not exist or is not visible to the caller. + */ + Domain detachFromParent() throws NoSuchDomainException; + + /** + * Replaces the current backing tag of this domain, or removes it, whatever backing tag this + * snapshot shows. Members are not migrated: objects that carry only the old backing tag are no + * longer members of the domain. Setting the current backing tag has no effect. + * + * @param backingTag The existing tag, with an optional value, that backs the domain, or null to + * remove the backing tag. + * @return The updated domain. + * @throws NoSuchDomainException If this domain does not exist or is not visible to the caller. + * @throws NoSuchTagException If the tag does not exist. + * @throws BackingTagAlreadyBoundException If the binding conflicts with the backing tag of + * another domain. + * @throws ForbiddenException If the caller does not own the tag or the metalake. Review Comment: [Important] Please make the domain-level authorization requirements part of the public API contract, rather than leaving them only in the design document. The mutation methods describe visibility and some tag privileges, but do not state that publish/unpublish/alter/delete require MANAGE_DOMAIN, attach/move requires it on both the domain and the new parent, and top-level creation requires the metalake-level privilege. The class-level inheritance description explains how privileges combine, but not which privilege each operation requires. There is also a concrete mismatch here: updateBackingTag(null) removes the binding, and the design requires only MANAGE_DOMAIN for that operation. This @throws description unconditionally requires ownership of the tag or metalake. Could we add the operation permission matrix (or equivalent method-level documentation), distinguish a hidden/nonexistent domain from a visible domain the caller cannot modify, and specify the corresponding failure signals? That gives external implementers and clients one consistent contract before the implementations arrive. ########## api/src/main/java/org/apache/gravitino/domain/DomainOperations.java: ########## @@ -0,0 +1,126 @@ +/* + * 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.domain; + +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.gravitino.annotation.Evolving; +import org.apache.gravitino.exceptions.BackingTagAlreadyBoundException; +import org.apache.gravitino.exceptions.DomainAlreadyExistsException; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.NoSuchDomainException; +import org.apache.gravitino.exceptions.NoSuchMetalakeException; +import org.apache.gravitino.exceptions.NoSuchTagException; +import org.apache.gravitino.exceptions.NonEmptyDomainException; +import org.apache.gravitino.exceptions.TagAlreadyExistsException; +import org.apache.gravitino.tag.TagValue; + +/** + * Interface for the domain operations of a metalake. Operations on a single domain, such as + * publishing it or managing its sub-domains, are on {@link Domain}. + * + * <p>A domain in {@link Domain.Status#DRAFT} status is returned only to the users who can manage + * it, including through its parent or the metalake (see {@link Domain}). For other users it is + * omitted from lists, and getting it throws {@link NoSuchDomainException}. + */ +@Evolving +public interface DomainOperations { + + /** + * Lists the names of the domains in the metalake, including sub-domains. + * + * @return The names of the domains. + * @throws NoSuchMetalakeException If the metalake does not exist. + */ + String[] listDomains() throws NoSuchMetalakeException; + + /** + * Lists the domains in the metalake with details, including sub-domains. + * + * @return The domains. + * @throws NoSuchMetalakeException If the metalake does not exist. + */ + Domain[] listDomainsInfo() throws NoSuchMetalakeException; + + /** + * Gets a domain by its name. + * + * @param name The name of the domain. + * @return The domain. + * @throws NoSuchDomainException If the domain does not exist or is not visible to the caller. + */ + Domain getDomain(String name) throws NoSuchDomainException; + + /** + * Creates a top-level domain in {@link Domain.Status#DRAFT} status. Use {@link + * Domain#createSubdomain(String, String, Map, TagValue, boolean)} to create a sub-domain. + * + * <p>The backing tag is always given explicitly. With {@code createBackingTag} set, a new tag Review Comment: [Important] Could we define the failure atomicity of createDomain(..., createBackingTag=true), and the corresponding createSubdomain operation? This API creates both a domain and a tag, but does not say whether the tag survives if domain creation fails. For example, a new tag could be created before the domain insert loses a concurrent name conflict; a retry could then fail with TagAlreadyExistsException even though the first request never created its domain. I suggest specifying that creation of the domain, its new backing tag, the parent edge where applicable, and their ownership records commits atomically; a failed create leaves no newly created tag or relation behind. When binding an existing tag, failure should leave that tag unchanged. This is a request to settle the public failure contract now, not a claim that the not-yet-added storage implementation already has this bug. -- 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]
