yuqi1129 commented on code in PR #13058: URL: https://github.com/apache/gravitino/pull/13058#discussion_r4021678181
########## 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: I would like to clarify the scope and target of this SPI before we settle its public contract. I see the motivation for integrating an external allocation service in the PR description. My remaining question is what resource this interface is responsible for, and where that responsibility ends relative to the table format. My understanding of the current implementation is: - `DefaultTableLocationProvider.provisionTableLocation()` only derives a location string from the table/schema/catalog properties. It does not create a directory or allocate storage, and its `unprovisionTableLocation()` is consequently a no-op. - For ordinary Lance creation, `LanceTableOperations` creates the dataset at the returned location. On managed-table drop, the format deletes the dataset before the provider receives the unprovision callback. - A custom provider may additionally call an external allocator and perform side effects, but that allocation is outside the built-in implementation. Could you make the intended boundary explicit, ideally with one concrete allocation/release example? 1. **What does provision promise?** Is returning a location sufficient, or must an allocating provider ensure that some backing resource already exists or is reserved before returning? If it creates a directory, bucket, or reservation, which of those resources belong to the provider, and which are created by the table format? A usable location, a storage reservation, and a created dataset are different outcomes; the contract should say which one callers can rely on. 2. **What exactly does unprovision release?** If provision only calculates a path, there is nothing to undo. If provision reserves storage or registers ownership/quota in an external service, then unprovision has a clear purpose: release that allocation. Is that the intended target? Please distinguish releasing allocator state or backing resources from deleting the table's dataset, which the format already handles on the relevant paths. Can unprovision itself delete physical storage, and if so, what establishes that the provider owns that storage and may safely reclaim it? Receiving a location string alone does not establish ownership, especially when the caller supplied it. 3. **What would a complete example look like?** For example: provision reserves a table-specific prefix and records an allocation; Lance creates and later deletes its dataset there; unprovision releases the reservation and associated accounting. Is this the actual use case, or does your allocator also create/delete backing resources? Please describe what remains to be released after the format has deleted the dataset. 4. **What lifecycle guarantee is in scope?** The documented lack of compensation after failed creation and the best-effort drop callback mean an allocating provider still needs independent reconciliation. That may be an acceptable boundary, but it should be stated as part of the feature's target: allocation hooks plus best-effort release notification, with recovery owned by the provider/deployment. I am not asking this PR to add a distributed transaction or a full recovery system. For a provider that only resolves paths, an empty unprovision makes sense. For a provider that manages allocations, the resource ownership and release semantics are the essential contract. Clarifying which problem this PR primarily targets would make it much easier to judge whether these two methods and their current guarantees are sufficient. -- 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]
