mchades commented on code in PR #14559:
URL: https://github.com/apache/gravitino/pull/14559#discussion_r4475385079
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/DomainMetaService.java:
##########
@@ -177,72 +185,179 @@ public void insertDomain(DomainEntity domainEntity,
boolean overwritten) throws
}
/**
- * Inserts a new domain after a check of the locked state. If the domain has
a parent, it is
- * inserted as a sub-domain of that parent, together with its parent edge,
in one transaction.
+ * Inserts a new domain backed by an existing tag, after a check of the
locked state. If the
+ * domain has a parent, it is inserted as a sub-domain of that parent,
together with its parent
+ * edge, in one transaction.
*
- * <p>The metalake row is locked shared, then the parent row, if any, is
locked shared, so the
- * parent cannot be attached elsewhere, deleted or have its status changed
until the domain is
- * committed. The check gets the new domain, the parent with its own parent
set, and no
- * sub-domains.
+ * <p>The metalake row is locked shared, then the parent row, if any,
shared, so the parent cannot
+ * be attached elsewhere, deleted or have its status changed until the
domain is committed. Then
+ * the tag is resolved by name and locked exclusively, so it cannot be
deleted or bound by another
+ * domain meanwhile; every binding of a tag holds this lock. The domain is
written next, so a
+ * missing parent or tag is reported before a duplicate domain name, and
then the check runs. The
+ * check gets the new domain with its backing tag set, the parent with its
own parent set, no
+ * sub-domains, the tag, and the domains the tag already backs.
*
* <p>This operation must own its transaction so a failed check rolls back
the inserted domain. It
* cannot run inside another transaction.
*
* @param domainEntity The domain to insert; its parent, if set, is the
parent to insert it under.
+ * @param backingTag The existing tag, with an optional value, that backs
the domain.
* @param check The check of the locked state; nothing is inserted if it
throws.
* @throws IOException If the domain cannot be stored.
* @throws EntityAlreadyExistsException If a domain with the same name
exists.
* @throws NoSuchEntityException If the metalake or the parent does not
exist.
* @throws IllegalStateException If it is called inside a transaction.
+ * @throws NoSuchTagException If the tag does not exist.
*/
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
- baseMetricName = "insertDomainWithCheck")
- public void insertDomain(DomainEntity domainEntity,
Consumer<LockedDomainState> check)
+ baseMetricName = "insertDomainWithBackingTag")
+ public void insertDomainWithBackingTag(
+ DomainEntity domainEntity, TagValue backingTag,
Consumer<LockedDomainState> check)
throws IOException {
- Preconditions.checkState(
- !SessionUtils.isInTransaction(),
- "The domain %s cannot be inserted with a check inside another
transaction",
- domainEntity.nameIdentifier());
- Namespace ns = domainEntity.namespace();
- String metalakeName = ns.level(0);
+ insertBackedDomain(
+ domainEntity,
+ backingTag.value().orElse(null),
+ metalakePO -> {
+ Namespace tagNamespace =
NamespaceUtil.ofTag(metalakePO.getMetalakeName());
+ TagEntity tag =
+ TagMetaService.getInstance()
+ .lockAndGetTagForDomainBinding(
+ metalakePO.getMetalakeId(), tagNamespace,
backingTag.name());
+ return Pair.of(
+ tag,
+ listDomainsByBackingTagId(
+ metalakePO.getMetalakeId(), tag.id(),
domainEntity.namespace()));
+ },
+ check,
+ null);
+ }
- try {
- MetalakePO metalakePO = getMetalakePO(metalakeName);
- DomainPO domainPO = DomainPO.initializeDomainPO(domainEntity,
metalakePO.getMetalakeId());
+ /**
+ * Inserts a new domain together with a new tag that backs it without a
value, after a check of
+ * the locked state. If the domain has a parent, it is inserted as a
sub-domain of that parent,
+ * together with its parent edge. The tag, the domain and the edge are
written in one transaction.
+ *
+ * <p>The metalake row is locked shared, then the parent row, if any,
shared. The new tag has no
+ * row to lock; the unique key on the tag name fails the insert if a tag
with the same name exists
+ * or is created at the same time. The check gets the new domain with its
backing tag set, the
+ * parent with its own parent set, no sub-domains, the new tag, and no other
domains.
+ *
+ * @param domainEntity The domain to insert; its parent, if set, is the
parent to insert it under.
+ * @param backingTag The new tag that backs the domain.
+ * @param check The check of the locked state; nothing is inserted if it
throws.
+ * @throws IOException If the domain cannot be stored.
+ * @throws EntityAlreadyExistsException If a domain with the same name
exists.
+ * @throws NoSuchEntityException If the metalake or the parent does not
exist.
+ * @throws TagAlreadyExistsException If a tag with the same name exists.
+ */
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "insertDomainWithNewBackingTag")
+ public void insertDomainWithNewBackingTag(
+ DomainEntity domainEntity, TagEntity backingTag,
Consumer<LockedDomainState> check)
+ throws IOException {
+ insertBackedDomain(
+ domainEntity,
+ null,
+ metalakePO -> Pair.of(backingTag, Collections.emptyList()),
+ check,
+ backingTag);
+ }
+ /**
+ * Sets or removes the backing tag of a domain, after a check of the locked
state. Setting the
+ * current backing tag writes nothing.
+ *
+ * <p>The metalake row is locked shared, then the domain row exclusively,
then the new tag, if
+ * any, exclusively, so the tag cannot be deleted or bound by another domain
until the change
+ * commits. The check gets the domain with its current parent and backing
tag set, no parent and
+ * no sub-domains, the new tag, and the domains the tag already backs, which
include this domain
+ * if the tag already backs it.
+ *
+ * @param identifier The identifier of the domain.
+ * @param backingTag The existing tag, with an optional value, that backs
the domain, or null to
+ * remove the backing tag.
+ * @param auditInfo The audit info of the change; the domain keeps its
creator and takes its last
+ * modifier.
+ * @param check The check of the locked state; nothing is written if it
throws.
+ * @return The domain after the change.
+ * @throws IOException If the domain cannot be stored.
+ * @throws NoSuchEntityException If the domain does not exist.
+ * @throws NoSuchTagException If the tag does not exist.
+ */
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "updateDomainBackingTagAndGet")
+ public DomainEntity updateDomainBackingTagAndGet(
+ NameIdentifier identifier,
+ @Nullable TagValue backingTag,
+ AuditInfo auditInfo,
+ Consumer<LockedDomainState> check)
+ throws IOException {
+ Namespace namespace = identifier.namespace();
+ String metalakeName = namespace.level(0);
+ DomainEntity[] updated = new DomainEntity[1];
+ try {
+ DomainPO observedPO = getDomainPOByMetalakeAndName(metalakeName,
identifier.name());
SessionUtils.doMultipleWithCommit(
- () ->
- lockMetalakeForDomainWrite(metalakePO.getMetalakeName(),
metalakePO.getMetalakeId()),
() -> {
- DomainEntity parent =
- domainEntity
- .parent()
- .map(
- parentName ->
- DomainRelService.getInstance()
-
.lockAndGetParentOfNewSubDomain(NameIdentifier.of(ns, parentName)))
- .orElse(null);
- // The domain is written before the check, so that a duplicate
name fails as such.
- insertNewDomainWithoutCommit(domainPO);
- check.accept(new LockedDomainState(domainEntity, parent,
Collections.emptyList()));
- if (parent != null) {
- DomainRelService.getInstance()
- .insertDomainRelOfNewSubDomain(
- metalakePO.getMetalakeId(),
- parent.id(),
- domainPO.domainId(),
- AuditInfo.builder()
- .withCreator(domainEntity.auditInfo().creator())
-
.withCreateTime(domainEntity.auditInfo().createTime())
- .build());
+ DomainPO lockedPO =
+
DomainRelService.getInstance().lockAndGetDomainForChange(identifier,
observedPO);
+ DomainEntity domain =
+ DomainRelService.getInstance().toEntityWithParent(lockedPO,
namespace);
+ TagEntity tag = null;
+ List<DomainEntity> bindings = Collections.emptyList();
+ if (backingTag != null) {
+ tag =
+ TagMetaService.getInstance()
+ .lockAndGetTagForDomainBinding(
+ lockedPO.metalakeId(),
+ NamespaceUtil.ofTag(metalakeName),
+ backingTag.name());
+ bindings = listDomainsByBackingTagId(lockedPO.metalakeId(),
tag.id(), namespace);
}
+ check.accept(
+ new LockedDomainState(domain, null, Collections.emptyList(),
tag, bindings));
+
+ Long newTagId = tag == null ? null : tag.id();
+ String newValue = backingTag == null ? null :
backingTag.value().orElse(null);
+ if (!Objects.equals(lockedPO.backingTagId(), newTagId)
+ || !Objects.equals(lockedPO.backingTagValue(), newValue)) {
+ DomainEntity newDomain =
+ domain.toBuilder()
+ .withBackingTagId(newTagId)
+ .withBackingTagName(tag == null ? null : tag.name())
+ .withBackingTagValue(newValue)
+ .withAuditInfo(
+ AuditInfo.builder()
+ .withCreator(domain.auditInfo().creator())
+ .withCreateTime(domain.auditInfo().createTime())
+ .withLastModifier(auditInfo.lastModifier())
+
.withLastModifiedTime(auditInfo.lastModifiedTime())
+ .build())
+ .build();
+ DomainPO newPO = DomainPO.updateDomainPO(lockedPO, newDomain);
+ OccWriteSupport.updateWithVersion(
+ () ->
+ SessionUtils.getWithoutCommit(
+ DomainMetaMapper.class,
+ mapper -> mapper.updateDomainMeta(newPO, lockedPO)),
+ () ->
+
ExceptionUtils.concurrentModification(Entity.EntityType.DOMAIN, identifier));
+ }
+ updated[0] =
+ fromDomainPOWithParent(
Review Comment:
For no-op updates (when neither `backingTagId` nor `backingTagValue` has
changed), we could directly assign `updated[0] = domain;` to avoid an
unnecessary database re-query within the transaction.
##########
core/src/main/java/org/apache/gravitino/domain/DomainManager.java:
##########
@@ -496,6 +589,44 @@ private static void checkParentName(String name, @Nullable
String parent) {
!Objects.equals(name, parent), "Domain %s cannot be its own parent",
name);
}
+ // Checks that the locked backing tag allows the value and that no other
domain has a binding of
+ // the tag that conflicts with it: a tag has either one binding without a
value, or bindings with
+ // values where each value backs at most one domain.
+ private static void checkBinding(
+ DomainEntity domain, Optional<String> value, LockedDomainState state) {
+ TagEntity tag = lockedBackingTag(state);
+ tag.validateAllowedValue(
+ value.map(v -> TagValue.of(tag.name(), v)).orElseGet(() ->
TagValue.noValue(tag.name())));
+
+ for (DomainEntity bound : state.backingTagBindings()) {
+ if (Objects.equals(bound.id(), domain.id())) {
+ continue;
+ }
+ if (bound.backingTagValue() == null) {
+ throw new BackingTagAlreadyBoundException(
Review Comment:
The validation strictly prevents mixing valueless bindings and valued
bindings for the same tag (a tag either has exactly one binding without value,
or distinct bindings each with a value). While this is well-covered by tests
and makes semantics unambiguous, it might be helpful to emphasize this mutual
exclusion rule in the class-level Javadoc of `DomainDispatcher` or
`DomainManager` to avoid user confusion.
##########
core/src/main/java/org/apache/gravitino/domain/DomainDispatcher.java:
##########
@@ -86,29 +95,39 @@ default String[] listDomains(String metalake) throws
NoSuchMetalakeException {
*/
DomainEntity getDomain(String metalake, String name) throws
NoSuchDomainException;
- // TODO: The next PR of #13884 adds the explicit backing tag to createDomain
and
- // createSubdomain, which every create request must name, and adds
updateBackingTag.
-
/**
- * Creates a top-level domain in draft status.
+ * Creates a top-level domain in draft status, backed by a new or an
existing tag.
*
* @param metalake The name of the metalake.
* @param name The name of the domain.
* @param comment The comment of the domain, or null.
* @param properties The properties of the domain, or null.
+ * @param backingTag The backing tag of the domain: the name of the tag to
create, or an existing
+ * tag with an optional value.
+ * @param createBackingTag Whether to create the backing tag as a new tag
without values.
* @return The created domain.
* @throws NoSuchMetalakeException If the metalake does not exist.
* @throws DomainAlreadyExistsException If a domain with the same name
exists.
+ * @throws NoSuchTagException If the existing backing tag to bind does not
exist.
+ * @throws TagAlreadyExistsException If the backing tag to create already
exists.
+ * @throws BackingTagAlreadyBoundException If the binding conflicts with the
backing tag of
+ * another domain.
+ * @throws IllegalArgumentException If the backing tag is null, a value is
given for a tag to
+ * create, or the value is not allowed by the existing tag.
*/
DomainEntity createDomain(
String metalake,
String name,
@Nullable String comment,
- @Nullable Map<String, String> properties)
- throws DomainAlreadyExistsException;
+ @Nullable Map<String, String> properties,
+ TagValue backingTag,
+ boolean createBackingTag)
Review Comment:
Consider using an enum (e.g. `BackingTagMode.CREATE_NEW` /
`BackingTagMode.BIND_EXISTING`) or dedicated dispatcher methods instead of a
boolean flag `createBackingTag` to improve readability at call sites.
##########
core/src/main/java/org/apache/gravitino/meta/TagEntity.java:
##########
@@ -134,6 +136,51 @@ public Optional<Boolean> inherited() {
return Optional.ofNullable(inherited);
}
+ /**
+ * Checks that this tag can be assigned with the given value. See {@link
+ * #validateAllowedValue(String[], TagValue)}.
+ *
+ * @param tagValue The tag value to assign.
+ * @throws IllegalArgumentException If this tag does not allow the value.
+ */
+ public void validateAllowedValue(TagValue tagValue) {
+ validateAllowedValue(allowedValues, tagValue);
+ }
+
+ /**
+ * Checks that a tag with the given allowed values can be assigned with a
value: a tag with
+ * allowed values only with one of them, a tag that allows no values only
without a value, and a
+ * tag without a constraint with or without any value.
+ *
+ * @param allowedValues The allowed values of the tag: null if the tag has
no constraint, empty if
+ * it allows no values.
+ * @param tagValue The tag value to assign.
+ * @throws IllegalArgumentException If the tag does not allow the value.
+ */
+ public static void validateAllowedValue(@Nullable String[] allowedValues,
TagValue tagValue) {
+ if (allowedValues == null) {
+ return;
+ }
+
+ if (!tagValue.value().isPresent()) {
+ Preconditions.checkArgument(
+ allowedValues.length == 0,
+ "Tag %s requires assignment values from allowed values %s",
+ tagValue.name(),
+ Arrays.toString(allowedValues));
+ return;
+ }
+
+ Preconditions.checkArgument(
+ allowedValues.length > 0, "Tag %s does not allow assignment values",
tagValue.name());
+ Preconditions.checkArgument(
+ Arrays.asList(allowedValues).contains(tagValue.value().get()),
Review Comment:
Nit: `Arrays.asList(allowedValues).contains(...)` allocates a wrapper list
per invocation. For larger allowed-values sets, using a `Set` or linear scan
would avoid the allocation. (Not blocking since this was moved from
`TagMetaService`).
--
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]
