shaoyu-li commented on code in PR #13058: URL: https://github.com/apache/gravitino/pull/13058#discussion_r4031552647
########## catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/TableLocationProvider.java: ########## @@ -0,0 +1,211 @@ +/* + * 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.catalog.lakehouse.generic; + +/** + * A pluggable strategy for deciding where the data of a newly created table lives. + * + * <p>Implementations are discovered through Java's {@link java.util.ServiceLoader} and selected by + * {@link #name()} using the {@code table-location-provider} catalog property. The built-in {@link + * DefaultTableLocationProvider} derives the location from the table, schema and catalog {@code + * location} properties; deployments that allocate storage through an external service can register + * their own implementation instead. + * + * <p>The interface is deliberately two operations wide: hand out a location for a table being + * created, and hand one back for a table that is gone. Every decision that can be made from what + * the catalog already knows is made by the catalog, so that an implementation has as little to get + * right as possible. In particular, whether an external table's data survives a drop is decided + * here and not there -- see {@link #unprovisionTableLocation(TableLocationContext)}. + * + * <p><b>There is no lifecycle.</b> An instance is created per catalog and is never initialized or + * closed by the catalog, so an implementation needing configuration of its own -- a service + * endpoint, a credential -- has to obtain it without help from here, and anything it acquires is + * held for the lifetime of the instance with no callback to release it. An implementation holding a + * remote client should therefore acquire it lazily and make it safe to abandon, because catalogs + * are evicted from the server's catalog cache when idle and a discarded provider is not told. + * + * <p>Implementations must be thread-safe: {@link #provisionTableLocation(TableLocationContext)} and + * {@link #unprovisionTableLocation(TableLocationContext)} are both called concurrently by table + * requests. A failure on the provisioning path fails the table creation; one on the drop path is + * logged at WARN and nothing else happens. + * + * <p>Implementations must satisfy two constraints imposed by the {@link java.util.ServiceLoader} + * based discovery: + * + * <ul> + * <li>They must have a public no-argument constructor that is cheap, does not throw and acquires + * nothing. Every provider registered on the classpath is instantiated before the one matching + * the catalog property is selected, so a heavy constructor slows the initialization of every + * catalog, including those using the built-in provider. One that throws is logged and skipped + * rather than failing the lookup, which costs that provider the ability to be selected at + * all. The instances that were not selected are then discarded, and nothing is closed on + * them, so anything a constructor acquires is leaked once per catalog creation. + * <li>{@link #name()} must be unique across the classpath, and must not be {@value + * DefaultTableLocationProvider#NAME}, which is reserved by {@link + * DefaultTableLocationProvider}. If two providers share a name, every catalog selecting that + * name fails to initialize. A provider whose constructor or {@link #name()} throws is logged + * and skipped rather than failing the lookup, so a provider that is broken at runtime does + * not stop catalogs that named a different one from starting. A services file naming a class + * that cannot be loaded at all still fails the lookup. + * </ul> + * + * <p><b>Known limitations.</b> Five of them, and they all point the same way: a provider that + * manages real storage needs its own reconciliation against the catalog and cannot treat the + * callbacks here as a complete record of what it handed out. + * + * <ul> + * <li>{@code location} is a mutable table property, so {@code alterTable(setProperty("location", + * ...))} repoints a table without this provider being told. The old location is never + * unprovisioned and the new one never went through this provider. + * <li>{@link #provisionTableLocation(TableLocationContext)} is called before the table is + * actually created, so a creation that fails afterwards -- a table that already exists, or a + * failure inside the table format itself -- leaves a location provisioned for a table that + * does not exist. There is no compensating unprovision, deliberately: a table format that + * fails partway through creation may already have written to the location, and calling {@link + * #unprovisionTableLocation(TableLocationContext)} would then tell the provider it is free to + * reclaim a path that has data on it. Leaking an unused path is the safer of the two + * failures, and doing better would need the format to report whether it touched storage + * before failing, which this interface cannot express. Every check this catalog can make on + * its own is made before the provider is consulted, so the cases that remain are the ones + * only the table format can detect. + * <li>The catalog decides that a provisioned location went unused by comparing it with the + * location the created table reports, ignoring a trailing slash, which is the one rewrite the + * catalog performs itself. A format that rewrites the location further -- collapsing a + * duplicated separator, or normalizing a URI scheme -- looks from here like a format that + * declined the location outright, so an implementation whose paths may come back rewritten + * should verify before reclaiming. + * <li>A table format that drops a table through its own internals, rather than through the + * catalog, does not trigger {@link #unprovisionTableLocation(TableLocationContext)}. Lance's + * {@code OVERWRITE} creation mode does this: it drops the existing table and creates a new + * one, so the old location is never handed back. A format-internal drop is not visible to the + * catalog, so this cannot be closed from here. + * <li>{@code alterTable(rename(...))} changes a table's identity without telling this provider, + * and without moving any data. A provider deriving the path from the table name is left with + * a path that no longer matches the name, which is cosmetic. A provider that books + * allocations against {@code (schema, table)} loses the table altogether: the drop that + * follows arrives under the new name, and the allocation booked under the old one is never + * handed back. Such a provider has to reconcile renames out of band, or the deployment has to + * forbid renaming tables in this catalog. + * </ul> + */ +public interface TableLocationProvider { Review Comment: Thank you for this -- working through it turned up that the contract was stating two opposite principles in two different places, which is very likely what prompted the question in the first place. bf947cde states one. **The catalog reports; the provider decides.** The catalog's part is to hand over an accurate account of what happened -- what the creation request asked for, where the table ended up, whether the data under a location is gone -- and the provider's part is to decide what that account means for the storage it manages. The catalog does not model what an implementation keeps, and an implementation is never asked to reconstruct what the catalog or the table formats did. There is exactly one exception, and it is now written as an exception rather than dressed up as the rule: a dropped external table does not reach the callback at all, because that is the single path where a provider acting on an accurate report could still destroy data irrecoverably. **The boundary you asked for.** A provider owns the right to use a path; the table format owns the content at the path. The dividing line is not physical against logical -- a provider may well create real infrastructure -- but what a thing was created for: anything brought into being so that the path can be used belongs to the provider, and anything written into the path belongs to the format. The catalog owns neither. It stores the location string verbatim and sequences the two calls. ### 1. What provision promises Exactly one thing: that the location is usable, meaning a table format may create its dataset there and will not be refused for any reason under the provider's control. Whether an implementation had to reserve, register or create anything to make that true is its own business, neither required nor forbidden by the contract. It does **not** promise that a directory or prefix exists, that a dataset exists, or that the location is empty. The last one is deliberate: in this catalog a caller usually supplies a location precisely because the data is already there. So of the three outcomes you listed, a caller can rely on the first only, and the contract now says that in those words. ### 2. What unprovision releases Releasing the allocation is the intended target, and this is now stated as a prohibition rather than only as a precondition: - It releases what the implementation issued: the reservation, the registry entry, whatever quota or grant it booked, and the name itself so that it can be handed out again. - It may remove a container it created itself -- a prefix or a bucket brought into being so the path could be used -- but only after establishing that the container holds nothing except what the implementation itself put there, and it leaves the container in place otherwise. - It must **not** delete content at the location. The table format owns the lifecycle of the data and has already removed it on every path that reaches the callback, so deleting anything further is at best redundant and at worst destroys data this catalog promised not to touch. On ownership you are right, and it was not only a gap in the wording. There was a call site where the catalog named a location the caller had supplied while the table was still alive: a creation whose format returned a pre-existing table, so the location provisioned for that call went unused and was handed back -- and with a request carrying its own `location`, that location is the caller's own path. b7c0aa6d added that call site; bf947cde removes it. Detecting "unused" meant comparing two location strings and concluding from the comparison what a provider had done internally, which is exactly the inference this interface should be leaving to the provider. It is recorded as a limitation instead, alongside the failures an allocating provider already has to reconcile. The callback is now reached in three situations, all of them after the table is gone, and the invariant it publishes -- nothing anyone needs is under the location named in the context -- is true again on all of them. The general form of your sentence is in the contract too: nothing about the location string establishes ownership on its own. The catalog supplies one fact, that nothing anyone needs is under that location any more; whether the implementation holds anything there is a fact only the implementation can establish, from its own records. ### 3. A complete example Your sketch is accurate, and the docs now carry it as a worked example. The one correction is that an allocator can create backing infrastructure, but coarser than a table: a container created on demand, shared by many tables, and never removed for one of them. Per table there are two things -- an entry in a registry keyed by the table's identity, and an empty directory marker under the path. What remains to be released after the format has deleted the dataset is therefore the registry entry, deleting which is also what frees the name so the path can be issued again, and that directory marker, removed only once nothing else is under the prefix. Neither is visible to the table format, and neither can be cleaned up by anything else. ### 4. Lifecycle guarantee Confirmed, and taken almost verbatim into the contract as the feature's scope: allocation hooks plus best-effort release notification, with recovery owned by the provider and the deployment. It is deliberately not a distributed transaction and not a recovery system. ### Which problem this targets Allocation management. Path derivation is what this catalog already did, preserved unchanged as the built-in provider, and it is the same contract with the allocation set empty: it composes a path from configuration, holds no reservation and no registry entry, and therefore has nothing to hand back, which is why its `unprovisionTableLocation` body is empty. So your closing paragraph is the answer rather than a fork in the road -- writing the contract for allocation management covers the path-resolving provider as the degenerate case, and the empty body is the correct implementation for that shape rather than an omission. One further limitation came out of this review: a provider deriving a deterministic path from a table's identity hands out the same path again when a same-named table is created after the old one is gone, and if anything survived under that path the next creation fails inside the table format and keeps failing, because every retry derives the same path. The built-in provider is one such provider. That is now documented rather than discovered later. -- 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]
