yuqi1129 commented on code in PR #13562:
URL: https://github.com/apache/gravitino/pull/13562#discussion_r4217221467
##########
api/src/main/java/org/apache/gravitino/SupportsCatalogs.java:
##########
@@ -51,7 +51,23 @@ public interface SupportsCatalogs {
* @return The list of catalog's information.
* @throws NoSuchMetalakeException If the metalake with namespace does not
exist.
*/
- Catalog[] listCatalogsInfo() throws NoSuchMetalakeException;
+ default Catalog[] listCatalogsInfo() throws NoSuchMetalakeException {
Review Comment:
Revisiting my earlier suggestion: turning `listCatalogsInfo()` from abstract
into a default that delegates to `listCatalogsInfo(true)`, whose default
throws, means an implementation that overrides neither method now compiles and
fails at runtime on the most basic call (it used to be a compile error). Could
we keep `listCatalogsInfo()` abstract and only add the new overload as a
default?
```java
Catalog[] listCatalogsInfo() throws NoSuchMetalakeException; // keep
abstract
/**
* ...
* @throws UnsupportedOperationException if the implementation cannot list
catalogs
* without properties
*/
default Catalog[] listCatalogsInfo(boolean includeProperties) throws
NoSuchMetalakeException {
throw new UnsupportedOperationException(...);
}
```
`GravitinoMetalake`/`GravitinoClient` already implement both, so nothing
else changes.
##########
api/src/main/java/org/apache/gravitino/SupportsCatalogs.java:
##########
@@ -51,7 +51,23 @@ public interface SupportsCatalogs {
* @return The list of catalog's information.
* @throws NoSuchMetalakeException If the metalake with namespace does not
exist.
*/
- Catalog[] listCatalogsInfo() throws NoSuchMetalakeException;
+ default Catalog[] listCatalogsInfo() throws NoSuchMetalakeException {
+ return listCatalogsInfo(true);
+ }
+
+ /**
+ * List all catalogs with their information in the metalake.
+ *
+ * <p>Implementations may omit catalog properties when {@code
includeProperties} is {@code false}.
Review Comment:
Could the Javadoc state the contract more precisely? When properties are
omitted:
- `properties()` returns an empty map (not null), so callers cannot tell it
apart from a catalog with no properties, and every property, including
`in-use`, is absent;
- name, type, provider, comment and audit info are still populated;
- the default implementation throws `UnsupportedOperationException` (please
add `@throws`).
##########
core/src/main/java/org/apache/gravitino/catalog/SupportsCatalogs.java:
##########
@@ -58,7 +58,25 @@ public interface SupportsCatalogs {
* @return The list of catalog's information.
* @throws NoSuchMetalakeException If the metalake with namespace does not
exist.
*/
- Catalog[] listCatalogsInfo(Namespace namespace) throws
NoSuchMetalakeException;
+ default Catalog[] listCatalogsInfo(Namespace namespace) throws
NoSuchMetalakeException {
Review Comment:
Same as the API interface: please keep `listCatalogsInfo(Namespace)`
abstract, and document the omitted-properties contract and `@throws
UnsupportedOperationException` on the new overload.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestCatalogOperations.java:
##########
@@ -177,7 +178,8 @@ public void testListCatalogsInfo() {
TestCatalog catalog1 = buildCatalog("metalake1", "catalog1");
TestCatalog catalog2 = buildCatalog("metalake1", "catalog2");
- when(manager.listCatalogsInfo(any())).thenReturn(new Catalog[] {catalog1,
catalog2});
+ when(manager.listCatalogsInfo(any(), anyBoolean()))
Review Comment:
With `anyBoolean()` here, nothing asserts that a `details=true` request
without `includeProperties` passes `true`. Flipping `@DefaultValue("true")` to
`"false"` is only caught incidentally by the later `verify(..., eq(false))`
call count. Could you add `verify(manager).listCatalogsInfo(any(), eq(true));`
after the first request to pin the backward-compatible default?
--
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]
