-
Notifications
You must be signed in to change notification settings - Fork 332
Introduce an option to add object storage prefix to table locations #1966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
Outdated
Show resolved
Hide resolved
|
Nice work on this piece. The approach looks good. Here are my two cents:
|
|
Good point @MonkeyCanCode re: the name. A previous version had a truly random prefix, but now it's deterministic based on a hash of the table identifier. I don't actually have a preference between the two or think it matters. Do you have a suggestion for a better name, though? Iceberg calls their similar feature Re (2), you're right to consider the risk of hash collisions esp. when we only use 20 bits. In fact, collisions are inevitable as you approach 1M identifiers. However MurmurHash exhibits strong avalanche properties, meaning even small differences in the identifier (like the A vs B) produce vastly different outputs. So while collisions are possible, the chance that structurally similar identifiers (e.g., namespaceA.tableX vs. namespaceB.tableX) collide in the 20-bit prefix is still low. As for uniqueness, since the full path still includes the actual identifier, there's no ambiguity or risk of overwriting. The sibling check looks at the full path. |
Thanks for the confirmation. |
|
But I really am open to changing the name @MonkeyCanCode -- I toyed with |
Yes, I think that is good. The one used by Iceberg is generic and not mentioned about random I think (as earlier they were using some random hash function as well...based on the code context, this one is derived from that, so I am assuming they changed to this hash as well). |
|
|
||
| public static final FeatureConfiguration<Boolean> DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED = | ||
| PolarisConfiguration.<Boolean>builder() | ||
| .key("DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it "DEFAULT"? It was not default in Polaris 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Iceberg code this method of computing table locations is apparently called an "object store" location provider 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Iceberg code this method of computing table locations is apparently called an "object store" location provider 🤔
Yes, it is only for the object store as they are flat and use those hashes for different compute on the backend to handle based on prefix (not a problem nor needed is backend is block storage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, the naming is really tough here. It's DEFAULT_LOCATION ... because it alters the "default location" given to a table if there is no user-specified location.
The Iceberg feature doesn't have this component because it applies within the user-specified table location, whereas the Polaris feature applies above the table location only if there isn't a user-specified location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about OBJECT_STORAGE_LOCATION_PREFIX_ENABLED?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not always applied like the Iceberg option, it's only applied when using the "default location" for a table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
* Ignore regenerate.sh on README.md (apache#1999) * OpenAPI-generate: Omit generation timestamp (apache#2004) The jaxrs-resteasy OpenAPI generator adds the generation timestamp to the generated sources by default. This behavior leads to different code for every generation, leading to unnecessary rebuilds (and re-tests), because the generated `.class` files are different. * Update CatalogEntity::Builder to set default CatalogType as INTERNAL (apache#1998) Encountered the issue while adding additional validations to `ExternalCatalog`. The `CatalogEntity::Builder` checks if the `Catalog::type` is set to `INTERNAL`, if not it defaults to `EXTERNAL`. However this is the opposite of the behavior defined in polaris-management-service.yml where the default is set to `INTERNAL`. This change only affects tests because in other cases the catalog entity is generated from the REST request. Testing: Updated CatalogEntityTest to ensure that the default is set to `INTERNAL`. * Support IMPLICIT authentication type for federated catalogs (apache#1925) Previously, the ConnectionConfigInfo required explicit AuthenticationParameters for every federated catalog. However, certain catalogs types that Polaris federates to (either now or in the future) allow `IMPLICIT` authentication, wherein the authentication parameters are picked from the environment or configuration files. This change enables federating to such catalogs without passing dummy secrets. The `IMPLICIT` option is guarded by the `SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES`. Hence users may create federated catalogs with `IMPLICIT` authentication only when the administrator explicitly enables this feature. * Fix helm doc (apache#2001) * Fix helm doc * Remove persistent ref * Remove persistent ref * Fixes based on feedback * Fixes based on feedback * Fixes based on feedback * Fixes based on feedback * feat(auth): Ability to override active roles provider per realm (apache#2000) * feat(auth): Ability to override active roles provider per realm * deprecate old property * add tests * Introduce an option to add object storage prefix to table locations (apache#1966) ### Problem Currently, Polaris enforces that the physical layout of entities maps to the logical layout: ``` catalog └── ns1 ├── ns2 │ └── table_b └── table_a ``` In the above example, the base locations of `table_a` and `ns2` are expected to be children of `ns1`, and the location of `table_b` is expected to be a child of `ns2`. This behavior is controlled by `ALLOW_UNSTRUCTURED_TABLE_LOCATION` and is the basis for the sibling overlap check when `OPTIMIZED_SIBLING_CHECK` is disabled or persistence cannot support the optimized check. However, some users have reported that this physical organization of data can lead to undesirable performance characteristics when hotspotting occurs across namespaces. If the underlying storage is range partitioned by key, this organization will tend to physically collocate logically-similar entities. ### Solution To solve this problem, this PR introduces a new option `DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED` which alters the behavior of the catalog when creating a table without a user-specified location. With the feature disabled, a table such as `ns1.table_a` will have a path like this: ``` s3://catalog/base/ns1/table_a/ ``` With the feature enabled, a prefix is added before the namespace: ``` s3://catalog/base/0010/0101/0110/10010100/ns1/table_a/ ``` This serves to eliminate the physical collocation of tables in the same namespace (or with similarly-named namespaces or table names). This functionality is similar to Iceberg's `write.object-storage.enabled`, but it applies across tables and namespaces. The two features can and should be combined to achieve the best distribution of data files throughout the key space. ### Configuration & Sibling Overlap Check If an admin doesn't care about the risk of vending credentials with the sibling overlap check disabled, they can enable the feature with these configs: ``` polaris.features.DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED=true polaris.features.ALLOW_UNSTRUCTURED_TABLE_LOCATION=true polaris.features.ALLOW_TABLE_LOCATION_OVERLAP=true polaris.behavior-changes.VALIDATE_VIEW_LOCATION_OVERLAP=false ``` In order to use this feature and to preserve the sibling overlap check, you can configure the service with: ``` polaris.features.DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLED=true polaris.features.ALLOW_UNSTRUCTURED_TABLE_LOCATION=true polaris.features.OPTIMIZED_SIBLING_CHECK=true ``` However, note that the `OPTIMIZED_SIBLING_CHECK` comes with some caveats as outlined in its description. Namely, it currently only works with some persistence implementations and it requires all location-based entities to have a recently-introduced field set. These locations are expected to be suffixed with `/`, and locations with many `/` may not be eligible for the optimized check. Older Polaris deployments may not meet these requirements without a migration or backfill. Accordingly combining these two features should be considered experimental for the time being. * Cleanup collaborators in `.asf.yaml` (apache#2008) Some devs were added in the past to `.asf.yaml` to let CI run w/o committer approval. After [INFRA-26985](https://issues.apache.org/jira/browse/INFRA-26985) this is no longer necessary, so the file can be cleaned up. * Fix bunch of OpenAPI generation issues (apache#2005) The current way how OpenAPI Java code is generated suffers from a bunch of issues: * Changes to any of the source spec files requires a Gradle `clean`, otherwise old generated Java source will remain - i.e. "no longer" existing sources are not removed. This is addressed by adding an additional action to `GenerateTask`. * The output of `GenerateTask` was explicitly not cached, this is removed, so the output is cached. * Add explicit inputs to `GenerateTask` to the whole templates and spec folders. * Restructure the download page (apache#2011) * Add 1.0.0-incubating release to the downloads page (apache#2018) * Add 1.0.0 docs to the huge menu (apache#2020) * Improve the bundle jar license and notice remove using exclude (apache#1991) * Remove duplicate MetaStoreManagerFactory mocks (apache#2023) also rename the field for clarity and consistency * Update Makefile for python client with auto setup (apache#1995) Automate python client setup and use a virtual env instead to avoid change an end-users' OS python * Add Helm Chart repo to the downloads page (apache#2025) * Publish helm doc (apache#2014) * Make PolarisConfiguration member variables private (apache#2007) * Make PolarisConfiguration members private * Make methods final * Use the 0.9.0 doc from the versioned-docs branch (apache#2026) * Helm key grouping and test cases (apache#2002) * Helm key grouping and test cases * Update README.md * Added backwards compatible * Fix conflict * Use coalesce instead of if else * Remove kind (apache#2028) * Remove kind * Remove k8 dir from check-md-link.yml * Sync helm doc (apache#2034) * Update release-guide.md for publishing docs (apache#2035) * [Site] Simplify the doc directory structure (apache#2033) * [Site] Update release-guide.md for release dir name (apache#2037) * Fix gralde command for helm image and remove simple-values.yaml (apache#2036) * Using the closer.lua download script (apache#2038) * Fix the LICENSE and NOTICE with the latest dependency updates (apache#1939) * Fix invalid redirect from public page (apache#2041) * Make StorageCredentialCache safe for mutli-realm usage (apache#2021) Injecting the request-scoped `RealmContext` into the application-scoped `StorageCredentialCache` makes things unnecessarily complicated. Similarly `StorageCredentialCacheKey` having a `@Nullable callContext` makes it more difficult to reason about. Instead we can determine all realm-specific values at the time of insertion (from the `PolarisCallContext` param of `getOrGenerateSubScopeCreds`). * feat(ci): Improve Gradle cache in CI (apache#1928) * Introduce RealmConfig (apache#2015) Getting a config value currently requires quite a ceremony: ``` ctx.getPolarisCallContext() .getConfigurationStore() .getConfiguration(ctx.getRealmContext(), "ALLOW_WILDCARD_LOCATION", false)) ``` since a `PolarisConfigurationStore` cant be used without a `RealmContext` it makes sense to add a dedicated interface. this allows removal of verbose code and also moves towards injecting that interface via CDI at a request/realm scope in the future. * Fix CI (apache#2043) The `store-gradle-cache` job in the `gradle.yaml` GitHub workflow is missing a "checkout", this change adds it to fix CI. * Fix CI (no 2) (apache#2044) The newly added `store-gradle-cache` CI job has run some Gradle task to trigger Gradle's automatic cache cleanup. In the source project Nessie we used a simple task `showVersion` to do this. As having this task in Polaris might be useful, adding this task as there's no other suitable task (cheap and not generating much output) seems legit. * Bump Quarkus version to unblock IntelliJ build (apache#1958) Use Quarkus 3.24.3 to fix build issues with `:polaris-server:classes` * Use application-scoped StorageCredentialCache (apache#2022) Since `StorageCredentialCache` is application scoped and after 6ddd148 its constructor no longer uses the `RealmContext` passed into `getOrCreateStorageCredentialCache` we can now let all `PolarisEntityManager` instances share the same `StorageCredentialCache`. * Attempt to make Renovate work again (apache#2052) Looks that I accidentally broke Renovate with apache#1891. This was made under the impression of the [Renovate change to support `baseBranches` in forking-renovate] (renovatebot/renovate#36054). However, a [later Renovate change](renovatebot/renovate#35579) seems to break that. The plan here is to: 1. remove the regex from our `baseBranches` option - if that doesn't work then 2. just use the default branch * main: Update actions/stale digest to 128b2c8 (apache#2053) * main: Update dependency com.azure:azure-sdk-bom to v1.2.36 (apache#2054) * main: Update dependency com.fasterxml.jackson:jackson-bom to v2.19.1 (apache#2055) * main: Update dependency com.google.cloud:google-cloud-storage-bom to v2.53.3 (apache#2057) * main: Update registry.access.redhat.com/ubi9/openjdk-21-runtime Docker tag to v1.22-1.1752066187 (apache#2059) * main: Update dependency com.github.ben-manes.caffeine:caffeine to v3.2.2 (apache#2056) * main: Update dependency gradle to v8.14.3 (main) (apache#2058) * main: Update dependency gradle to v8.14.3 * Adjust Gradle update --------- Co-authored-by: Robert Stupp <[email protected]> * main: Update dependency io.micrometer:micrometer-bom to v1.15.2 (apache#2063) * main: Update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.1.0 (apache#2067) * main: Update dependency com.nimbusds:nimbus-jose-jwt to v10.3.1 (apache#2062) * main: Update docker.io/prom/prometheus Docker tag to v3.5.0 (apache#2071) * main: Update dependency org.junit:junit-bom to v5.13.3 (apache#2064) * main: Update docker.io/jaegertracing/all-in-one Docker tag to v1.71.0 (apache#2070) * main: Update medyagh/setup-minikube action to v0.0.20 (apache#2066) * main: Update dependency org.apache.commons:commons-lang3 to v3.18.0 (apache#2069) * main: Update log4j2 monorepo to v2.25.1 (apache#2073) * main: Update immutables to v2.11.0 (apache#2072) * main: Update dependency org.testcontainers:testcontainers-bom to v1.21.3 (apache#2065) * main: Update dependency com.google.errorprone:error_prone_core to v2.40.0 (apache#2068) * main: Update dependency io.netty:netty-codec-http2 to v4.2.3.Final (apache#2074) * main: Update dependency io.prometheus:prometheus-metrics-exporter-servlet-jakarta to v1.3.10 (apache#2076) * main: Update dependency net.ltgt.gradle:gradle-errorprone-plugin to v4.3.0 (apache#2079) * main: Update dependency io.projectreactor.netty:reactor-netty-http to v1.2.8 (apache#2075) * main: Update dependency com.gradleup.shadow:shadow-gradle-plugin to v8.3.8 (apache#2061) * main: Update dependency org.eclipse.persistence:eclipselink to v4.0.7 (apache#2078) * Add External Identity Providers page to unreleased documentation (apache#2013) --------- Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Eric Maynard <[email protected]> * main: Update dependency io.opentelemetry:opentelemetry-bom to v1.52.0 (apache#2082) * main: Update dependency software.amazon.awssdk:bom to v2.31.78 (apache#2080) * main: Update dependency com.adobe.testing:s3mock-testcontainers to v4.6.0 (apache#2081) * main: Update dependency io.smallrye.common:smallrye-common-annotation to v2.13.7 (apache#2083) * Revert PR 2033 (apache#2087) The PR apache#2033 was merged within less than 3 hours, late on a Friday. Since that change does not address an issue that seriously deserves a quick reaction nor is it a "nit", I'm proposing to revert the change. We do have [community best practices](https://polaris.apache.org/community/contributing-guidelines/) stating to give the whole community enough time to review, which did not happen. There are concerns that the PR apache#2033 will interfere with the whole effort to automate releases. Since there was no change to review and raise the concerns, I'd like to revert it to not cause any friction with that bigger effort. Revert "Fix invalid redirect from public page (apache#2041)", commit 493bc8e. Revert "[Site] Simplify the doc directory structure (apache#2033)", commit 2db2f10. * Renovate PRs, branch name + PR subject (apache#2060) Until June, Renovate PRs behaved a little bit different than today. The difference is the branch name. Before it was something like `renovate-bot/renovate/main/org.openapi.generator-7.x`, now it's like `renovate-bot/renovate/main-main/actions-stale-digest` (branch name is duplicated). I also noticed that the branch name is repeated in the PR subject, which started to be that way some longer ago. This change removes both duplications. * Simplify RealmEntityManagerFactory usage in tests (apache#2050) since all ctor params are created in `IcebergCatalogTest.before` we can do the same for `RealmEntityManagerFactory` `PolarisAuthzTestBase.entityManager` is already getting derived from `realmEntityManagerFactory`: https://github.com/apache/polaris/blob/2c2052c28f899aaa85e5f11a9131d9812ec62679/runtime/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java#L247 * Use PolarisImmutable for StorageCredentialCacheKey (apache#2029) * remove unused entityId from StorageCredentialCacheKey * convert StorageCredentialCacheKey to immutables * Disable renovatebot on release branches (apache#2085) Per the mailing list thread "[DISCUSS] Disable renovatebot on release branches", we should not do automatic dependency upgrades for release branches. Since it seems `release/1.0.x` is a release branch, we can remove this regex from renovate's list. * Site: Remove non-OSS query engines from front page (apache#2031) * update query engines list * Add Dremio OSS * fix(deps): update immutables to v2.11.1 (apache#2113) * fix(deps): update dependency boto3 to v1.39.4 (apache#2116) * chore: Avoid deprecated `DefaultCredentialsProvider.create()` (apache#2119) Use `DefaultCredentialsProvider.builder().build()` as suggested by AWS SDK javadoc. * fix(deps): update dependency boto3 to v1.39.6 (apache#2120) * Extensible pagination token implementation (apache#1938) Based on apache#1838, following up on apache#1555 * Allows multiple implementations of `Token` referencing the "next page", encapsulated in `PageToken`. No changes to `polaris-core` needed to add custom `Token` implementations. * Extensible to (later) support (cryptographic) signatures to prevent tampered page-token * Refactor pagination code to delineate API-level page tokens and internal "pointers to data" * Requests deal with the "previous" token, user-provided page size (optional) and the previous request's page size. * Concentrate the logic of combining page size requests and previous tokens in `PageTokenUtil` * `PageToken` subclasses are no longer necessary. * Serialzation of `PageToken` uses Jackson serialization (smile format) Since no (metastore level) implementation handling pagination existed before, no backwards compatibility is needed. Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Eric Maynard <[email protected]> * Site/dev: allow overriding the podman/docker binaries detection (apache#2051) The scripts in the `bin/` directory are built to work with both Docker and podman. There are nuances in how both behave, especially wrt docker/podman-compose. Some local environment specifics require the use of `podman-compose`, others the use of `docker-compose`. The default behavior is to prefer the `podman` and `podman-compose` binaries, if those exist and fall back to `docker` and `docker-compose`. Some setups using podman however require the use of `docker-compose` even if `podman-compose` is installed. This may manifest in an error message stating that `--userns` and `--pod` cannot be used together. In that case create a file `.user-settings` in the `site/` folder and add these two lines: ```bash DOCKER=docker COMPOSE=docker-compose ``` * NoSQL: build descriptions * NoSQL: README nits * NoSQL: Misc ports * Pagination * Policy fixes * Adoptions to "conflicting" changes * runtime-service test abstractions * Last merged commit d2667e5 --------- Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Pooja Nilangekar <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Yun Zou <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Dongjoon Hyun <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Adnan Hemani <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Mark Hoerth <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Danica Fine <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Honah (Jonas) J. <[email protected]>
Problem
Currently, Polaris enforces that the physical layout of entities maps to the logical layout:
In the above example, the base locations of
table_aandns2are expected to be children ofns1, and the location oftable_bis expected to be a child ofns2.This behavior is controlled by
ALLOW_UNSTRUCTURED_TABLE_LOCATIONand is the basis for the sibling overlap check whenOPTIMIZED_SIBLING_CHECKis disabled or persistence cannot support the optimized check.However, some users have reported that this physical organization of data can lead to undesirable performance characteristics when hotspotting occurs across namespaces. If the underlying storage is range partitioned by key, this organization will tend to physically collocate logically-similar entities.
Solution
To solve this problem, this PR introduces a new option
DEFAULT_LOCATION_OBJECT_STORAGE_PREFIX_ENABLEDwhich alters the behavior of the catalog when creating a table without a user-specified location. With the feature disabled, a table such asns1.table_awill have a path like this:With the feature enabled, a prefix is added before the namespace:
This serves to eliminate the physical collocation of tables in the same namespace (or with similarly-named namespaces or table names).
This functionality is similar to Iceberg's
write.object-storage.enabled, but it applies across tables and namespaces. The two features can and should be combined to achieve the best distribution of data files throughout the key space.Configuration & Sibling Overlap Check
If an admin doesn't care about the risk of vending credentials with the sibling overlap check disabled, they can enable the feature with these configs:
In order to use this feature and to preserve the sibling overlap check, you can configure the service with:
However, note that the
OPTIMIZED_SIBLING_CHECKcomes with some caveats as outlined in its description. Namely, it currently only works with some persistence implementations and it requires all location-based entities to have a recently-introduced field set. These locations are expected to be suffixed with/, and locations with many/may not be eligible for the optimized check.Older Polaris deployments may not meet these requirements without a migration or backfill. Accordingly combining these two features should be considered experimental for the time being.