flyrain commented on code in PR #2422:
URL: https://github.com/apache/polaris/pull/2422#discussion_r2299806335
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -671,6 +676,11 @@ public boolean setProperties(Namespace namespace,
Map<String, String> properties
} else {
LOGGER.debug("Skipping location overlap validation for namespace '{}'",
namespace);
}
+ if (!realmConfig.getConfig(
+ FeatureConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION, catalogEntity)) {
+ LOGGER.debug("Validating that namespace {} has a location inside its
parent", namespace);
Review Comment:
Same here
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1083,6 +1093,74 @@ private void validateNoLocationOverlap(
}
}
+ /** Checks whether the location of a namespace is valid given its parent */
+ private void validateNamespaceLocation(
+ NamespaceEntity namespace, PolarisResolvedPathWrapper resolvedParent) {
+ StorageLocation namespaceLocation =
+ StorageLocation.of(
+ StorageLocation.ensureTrailingSlash(
+ resolveNamespaceLocation(namespace.asNamespace(),
namespace.getPropertiesAsMap())));
+ PolarisEntity parent = resolvedParent.getResolvedLeafEntity().getEntity();
+ if (parent.getType().equals(PolarisEntityType.CATALOG)) {
+ CatalogEntity parentEntity = CatalogEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent catalog {}",
+ namespace.getName(),
+ parentEntity.getName());
+ var storageConfigInfo = parentEntity.getStorageConfigurationInfo();
+ if (storageConfigInfo == null) {
+ throw new IllegalArgumentException(
+ "Cannot create namespace without a parent storage configuration");
+ }
+ List<StorageLocation> defaultLocations =
+
parentEntity.getStorageConfigurationInfo().getAllowedLocations().stream()
+ .filter(java.util.Objects::nonNull)
+ .map(
+ l -> {
+ return StorageLocation.ensureTrailingSlash(
+ StorageLocation.ensureTrailingSlash(l) +
namespace.getName());
+ })
Review Comment:
minor:
```suggestion
l ->
StorageLocation.ensureTrailingSlash(
StorageLocation.ensureTrailingSlash(l) +
namespace.getName())
)
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -508,6 +508,11 @@ private void createNamespaceInternal(
} else {
LOGGER.debug("Skipping location overlap validation for namespace '{}'",
namespace);
}
+ if (!realmConfig.getConfig(
+ FeatureConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION, catalogEntity)) {
Review Comment:
Why do we validate the namespace location when
`ALLOW_NAMESPACE_CUSTOM_LOCATION` is false? I think we should validate it when
we allow customer location.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -508,6 +508,11 @@ private void createNamespaceInternal(
} else {
LOGGER.debug("Skipping location overlap validation for namespace '{}'",
namespace);
}
+ if (!realmConfig.getConfig(
+ FeatureConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION, catalogEntity)) {
+ LOGGER.debug("Validating that namespace {} has a location inside its
parent", namespace);
Review Comment:
Minor: is it necessary to have another debugging message here given line
1106 provides a similar one?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1083,6 +1093,74 @@ private void validateNoLocationOverlap(
}
}
+ /** Checks whether the location of a namespace is valid given its parent */
+ private void validateNamespaceLocation(
+ NamespaceEntity namespace, PolarisResolvedPathWrapper resolvedParent) {
+ StorageLocation namespaceLocation =
+ StorageLocation.of(
+ StorageLocation.ensureTrailingSlash(
+ resolveNamespaceLocation(namespace.asNamespace(),
namespace.getPropertiesAsMap())));
+ PolarisEntity parent = resolvedParent.getResolvedLeafEntity().getEntity();
+ if (parent.getType().equals(PolarisEntityType.CATALOG)) {
+ CatalogEntity parentEntity = CatalogEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent catalog {}",
+ namespace.getName(),
+ parentEntity.getName());
+ var storageConfigInfo = parentEntity.getStorageConfigurationInfo();
+ if (storageConfigInfo == null) {
+ throw new IllegalArgumentException(
+ "Cannot create namespace without a parent storage configuration");
+ }
+ List<StorageLocation> defaultLocations =
+
parentEntity.getStorageConfigurationInfo().getAllowedLocations().stream()
+ .filter(java.util.Objects::nonNull)
+ .map(
+ l -> {
+ return StorageLocation.ensureTrailingSlash(
+ StorageLocation.ensureTrailingSlash(l) +
namespace.getName());
+ })
+ .map(StorageLocation::of)
+ .toList();
+ if (!defaultLocations.contains(namespaceLocation)) {
+ throw new IllegalArgumentException(
+ "Namespace "
+ + namespace.getName()
+ + " has a custom location, "
+ + "which is not enabled. Expected a location in: ["
+ + String.join(
+ ", ",
defaultLocations.stream().map(StorageLocation::toString).toList())
+ + "]");
+ }
+ } else if (parent.getType().equals(PolarisEntityType.NAMESPACE)) {
+ NamespaceEntity parentEntity = NamespaceEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent namespace {}",
+ namespace.getName(),
+ parentEntity.getName());
+ String parentLocation =
+ resolveNamespaceLocation(parentEntity.asNamespace(),
parentEntity.getPropertiesAsMap());
+ StorageLocation defaultLocation =
+ StorageLocation.of(
+ StorageLocation.ensureTrailingSlash(
+ StorageLocation.ensureTrailingSlash(parentLocation) +
namespace.getName()));
+ if (!defaultLocation.equals(namespaceLocation)) {
+ throw new IllegalArgumentException(
+ "Namespace "
+ + namespace.getName()
+ + " has a custom location, "
+ + "which is not enabled. Expected location: "
Review Comment:
```suggestion
+ "which is not enabled. Expected a location within: "
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -508,6 +508,11 @@ private void createNamespaceInternal(
} else {
LOGGER.debug("Skipping location overlap validation for namespace '{}'",
namespace);
}
+ if (!realmConfig.getConfig(
+ FeatureConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION, catalogEntity)) {
+ LOGGER.debug("Validating that namespace {} has a location inside its
parent", namespace);
+ validateNamespaceLocation(entity, resolvedParent);
+ }
Review Comment:
Minor: It'd be nice to wrap this into a method, so that we don't duplicate
them in two different places.
```
if
(!realmConfig.getConfig(FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP))
{
LOGGER.debug("Validating no overlap for {} with sibling tables or
namespaces", namespace);
validateNoLocationOverlap(entity, resolvedParent.getRawFullPath());
} else {
LOGGER.debug("Skipping location overlap validation for namespace
'{}'", namespace);
}
if (!realmConfig.getConfig(
FeatureConfiguration.ALLOW_NAMESPACE_CUSTOM_LOCATION,
catalogEntity)) {
LOGGER.debug("Validating that namespace {} has a location inside its
parent", namespace);
validateNamespaceLocation(entity, resolvedParent);
}
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1083,6 +1093,74 @@ private void validateNoLocationOverlap(
}
}
+ /** Checks whether the location of a namespace is valid given its parent */
+ private void validateNamespaceLocation(
+ NamespaceEntity namespace, PolarisResolvedPathWrapper resolvedParent) {
+ StorageLocation namespaceLocation =
+ StorageLocation.of(
+ StorageLocation.ensureTrailingSlash(
+ resolveNamespaceLocation(namespace.asNamespace(),
namespace.getPropertiesAsMap())));
+ PolarisEntity parent = resolvedParent.getResolvedLeafEntity().getEntity();
+ if (parent.getType().equals(PolarisEntityType.CATALOG)) {
+ CatalogEntity parentEntity = CatalogEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent catalog {}",
+ namespace.getName(),
+ parentEntity.getName());
+ var storageConfigInfo = parentEntity.getStorageConfigurationInfo();
+ if (storageConfigInfo == null) {
+ throw new IllegalArgumentException(
+ "Cannot create namespace without a parent storage configuration");
+ }
+ List<StorageLocation> defaultLocations =
+
parentEntity.getStorageConfigurationInfo().getAllowedLocations().stream()
+ .filter(java.util.Objects::nonNull)
+ .map(
+ l -> {
+ return StorageLocation.ensureTrailingSlash(
+ StorageLocation.ensureTrailingSlash(l) +
namespace.getName());
+ })
+ .map(StorageLocation::of)
+ .toList();
+ if (!defaultLocations.contains(namespaceLocation)) {
+ throw new IllegalArgumentException(
+ "Namespace "
+ + namespace.getName()
+ + " has a custom location, "
+ + "which is not enabled. Expected a location in: ["
+ + String.join(
+ ", ",
defaultLocations.stream().map(StorageLocation::toString).toList())
+ + "]");
+ }
+ } else if (parent.getType().equals(PolarisEntityType.NAMESPACE)) {
+ NamespaceEntity parentEntity = NamespaceEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent namespace {}",
+ namespace.getName(),
+ parentEntity.getName());
+ String parentLocation =
+ resolveNamespaceLocation(parentEntity.asNamespace(),
parentEntity.getPropertiesAsMap());
+ StorageLocation defaultLocation =
+ StorageLocation.of(
+ StorageLocation.ensureTrailingSlash(
+ StorageLocation.ensureTrailingSlash(parentLocation) +
namespace.getName()));
+ if (!defaultLocation.equals(namespaceLocation)) {
+ throw new IllegalArgumentException(
+ "Namespace "
+ + namespace.getName()
+ + " has a custom location, "
+ + "which is not enabled. Expected location: "
+ + defaultLocation);
+ }
+ } else {
+ throw new IllegalArgumentException(
+ "Failed to validate namespace "
+ + namespace.getName()
+ + " given parent "
+ + parent.getName());
+ }
Review Comment:
Minor: the logic would be slight easier to understand if we add something
like this in the beginning of the method
```
Preconditions.checkArgument(parentType.equals(PolarisEntityType.CATALOG)
|| parentType.equals(PolarisEntityType.NAMESPACE), "Invalid parent type");
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1083,6 +1093,74 @@ private void validateNoLocationOverlap(
}
}
+ /** Checks whether the location of a namespace is valid given its parent */
+ private void validateNamespaceLocation(
+ NamespaceEntity namespace, PolarisResolvedPathWrapper resolvedParent) {
+ StorageLocation namespaceLocation =
+ StorageLocation.of(
+ StorageLocation.ensureTrailingSlash(
+ resolveNamespaceLocation(namespace.asNamespace(),
namespace.getPropertiesAsMap())));
+ PolarisEntity parent = resolvedParent.getResolvedLeafEntity().getEntity();
+ if (parent.getType().equals(PolarisEntityType.CATALOG)) {
+ CatalogEntity parentEntity = CatalogEntity.of(parent);
+ LOGGER.debug(
+ "Validating namespace {} given parent catalog {}",
+ namespace.getName(),
+ parentEntity.getName());
+ var storageConfigInfo = parentEntity.getStorageConfigurationInfo();
+ if (storageConfigInfo == null) {
+ throw new IllegalArgumentException(
+ "Cannot create namespace without a parent storage configuration");
+ }
+ List<StorageLocation> defaultLocations =
+
parentEntity.getStorageConfigurationInfo().getAllowedLocations().stream()
+ .filter(java.util.Objects::nonNull)
+ .map(
+ l -> {
+ return StorageLocation.ensureTrailingSlash(
+ StorageLocation.ensureTrailingSlash(l) +
namespace.getName());
+ })
+ .map(StorageLocation::of)
+ .toList();
+ if (!defaultLocations.contains(namespaceLocation)) {
+ throw new IllegalArgumentException(
+ "Namespace "
+ + namespace.getName()
+ + " has a custom location, "
Review Comment:
Can we output the invalid custom location here to make debugging a bit
easier?
--
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]
