[#8892] feat(Lance-REST-Server): implement namespace operation APIs for LRS#8902
Conversation
|
@beinan cc |
| public DescribeNamespaceResponse describeNamespace(String namespaceId, String delimiter) { | ||
| ObjectIdentifier nsId = ObjectIdentifier.of(namespaceId, delimiter); | ||
| Preconditions.checkArgument( | ||
| nsId.levels() <= 2, "Expected at most 2-level namespace but got: %s", namespaceId); |
There was a problem hiding this comment.
Shall we check that the namespace exactly match 2 levels?
There was a problem hiding this comment.
Because there is also a location property on catalog, if it is limited to exact match of 2 levels, then the property on catalog cannot be viewed through this API.
| break; | ||
| case 1: | ||
| String schemaName = nsId.levelAtListPos(1); | ||
| Schema schema = catalog.asSchemas().loadSchema(schemaName); |
There was a problem hiding this comment.
Shall we handle the exception like schema not found.
There was a problem hiding this comment.
The class org.apache.gravitino.lance.service.LanceExceptionMapper has already handled this
| } | ||
| if (!isLakehouseCatalog(catalog)) { | ||
| throw LanceNamespaceException.notFound( | ||
| "Catalog not found: " + catalogName, |
There was a problem hiding this comment.
The exception log is not correct.
There was a problem hiding this comment.
My initial thought is that we assume only the Lakehouse catalog is visible to the Lance Rest Server. So, if a catalog exists but is not a Lakehouse catalog, it would be considered a nonexistent catalog by the LRS.
There was a problem hiding this comment.
I think we can treat as not found exception, but the detailed message should be clarified, otherwise it is a little misleading.
| response.setProperties( | ||
| createdSchema.properties() == null ? Maps.newHashMap() : createdSchema.properties()); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Shall we catch other expcetions here and above?
There was a problem hiding this comment.
unnecessary, they will be handled in the server layer
| default: | ||
| throw new IllegalArgumentException("Unknown mode: " + mode); | ||
| } | ||
| } catch (NoSuchSchemaException e) { |
There was a problem hiding this comment.
We'd also change the code structure like above for catalog.
|
LGTM, just one comment. |
…APIs for LRS (apache#8902) ### What changes were proposed in this pull request? implement namespace operation APIs for LRS ### Why are the changes needed? Fix: apache#8892 ### Does this PR introduce _any_ user-facing change? yes, new REST APIs added ### How was this patch tested? not now
…APIs for LRS (apache#8902) implement namespace operation APIs for LRS Fix: apache#8892 yes, new REST APIs added not now
2 participants
What changes were proposed in this pull request?
implement namespace operation APIs for LRS
Why are the changes needed?
Fix: #8892
Does this PR introduce any user-facing change?
yes, new REST APIs added
How was this patch tested?
not now