shaoyu-li commented on PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#issuecomment-5675484360
Pushed 622788ea and b7c0aa6d, which narrow the SPI rather than adding to it.
Following our design discussion,
the interface is now two operations:
```java
public interface TableLocationProvider {
String name();
String provisionTableLocation(TableLocationContext context);
void unprovisionTableLocation(TableLocationContext context);
}
```
Removed, with the reasoning being the same one in each case -- the catalog
was already making the
decision and then handing the provider what it needed to make it again:
- `initialize(Map)` and `close()`. A provider is never initialized and never
closed. Configuration
arrives as `TableLocationContext#catalogProperties()`, which the built-in
provider needs anyway
for the catalog-level step of its fallback chain. A provider holding a
remote client should
create it lazily and be safe to abandon.
- `releaseUnusedLocation`. A creation the format serves from an existing
table still has to hand
back the location provisioned for that call, but it does not need a
callback of its own:
`unprovisionTableLocation` already asks for the release of the location
named in the context, and
that is exactly what this is. The catalog now calls it with the
*provisioned* location, not the
one the table ended up at. The contract says the consequence plainly --
the callback is reached
in four situations rather than three, and in the fourth the table still
exists, which is safe for
an implementation that releases the named location and unsafe only for one
that deletes whatever
it has booked against the table identity, which no path here asks for.
- `TableLocationContext#isExternal()`, and `isPurge()` in the previous
commit. The drop-versus-purge
and external decisions are made in the catalog, which reads `external` the
way the table formats
read it. The flag is still there in `tableProperties()` for a provider
that wants it.
One side effect worth noting: `GenericCatalogOperations#close()` is back to
its original signature,
so this PR no longer widens any public Java method. The only user-facing
behaviour change left is
the `404` → `200 {"dropped": false}` on dropping a missing table.
Tests: 92 in the module, 0 failures, 49 new. New this round: the catalog
properties reaching the
provider end to end, the built-in provider reading the catalog location per
call rather than
capturing it, and the unused location being handed back with the provisioned
path rather than the
stored one. I checked each behavioural assertion by reverting the code it
covers and confirming the
test fails, which is also how I found that an earlier revision's
unused-location detection had no
live coverage left at all.
The description is rewritten to match. Ready for another look.
--
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]
