Copilot commented on code in PR #11113:
URL: https://github.com/apache/gravitino/pull/11113#discussion_r3577208260
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceNamespaceWrapper.java:
##########
@@ -135,4 +169,400 @@ public Catalog loadAndValidateLakehouseCatalog(String
catalogName) {
}
return catalog;
}
+
+ String[] listSchemas(Catalog catalog) throws NoSuchCatalogException {
+ SchemaDispatcher schemaDispatcher = currentSchemaDispatcher();
+ if (schemaDispatcher != null) {
+ return
Arrays.stream(schemaDispatcher.listSchemas(Namespace.of(metalakeName,
catalog.name())))
+ .map(NameIdentifier::name)
+ .toArray(String[]::new);
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof SupportsSchemas) {
+ return Arrays.stream(((SupportsSchemas)
ops).listSchemas(Namespace.of()))
+ .map(NameIdentifier::name)
+ .toArray(String[]::new);
+ }
+ }
+
+ return catalog.asSchemas().listSchemas();
+ }
+
+ boolean schemaExists(Catalog catalog, String schemaName) {
+ SchemaDispatcher schemaDispatcher = currentSchemaDispatcher();
+ if (schemaDispatcher != null) {
+ return schemaDispatcher.schemaExists(schemaIdent(catalog.name(),
schemaName));
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof SupportsSchemas) {
+ return ((SupportsSchemas)
ops).schemaExists(NameIdentifier.of(schemaName));
+ }
+ }
+
+ return catalog.asSchemas().schemaExists(schemaName);
+ }
+
+ Schema loadSchema(Catalog catalog, String schemaName) {
+ SchemaDispatcher schemaDispatcher = currentSchemaDispatcher();
+ if (schemaDispatcher != null) {
+ return schemaDispatcher.loadSchema(schemaIdent(catalog.name(),
schemaName));
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof SupportsSchemas) {
+ return ((SupportsSchemas)
ops).loadSchema(NameIdentifier.of(schemaName));
+ }
+ }
+
+ return catalog.asSchemas().loadSchema(schemaName);
+ }
+
+ Schema createSchema(
+ Catalog catalog, String schemaName, String comment, Map<String, String>
properties) {
+ SchemaDispatcher schemaDispatcher = currentSchemaDispatcher();
+ if (schemaDispatcher != null) {
+ return schemaDispatcher.createSchema(
+ schemaIdent(catalog.name(), schemaName), comment, properties);
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof SupportsSchemas) {
+ return ((SupportsSchemas) ops)
+ .createSchema(NameIdentifier.of(schemaName), comment, properties);
+ }
+ }
+
+ return catalog.asSchemas().createSchema(schemaName, comment, properties);
+ }
+
+ Schema alterSchema(Catalog catalog, String schemaName, SchemaChange...
changes) {
+ SchemaDispatcher schemaDispatcher = currentSchemaDispatcher();
+ if (schemaDispatcher != null) {
+ return schemaDispatcher.alterSchema(schemaIdent(catalog.name(),
schemaName), changes);
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof SupportsSchemas) {
+ return ((SupportsSchemas)
ops).alterSchema(NameIdentifier.of(schemaName), changes);
+ }
+ }
+
+ return catalog.asSchemas().alterSchema(schemaName, changes);
+ }
+
+ boolean dropSchema(Catalog catalog, String schemaName, boolean cascade) {
+ SchemaDispatcher schemaDispatcher = currentSchemaDispatcher();
+ if (schemaDispatcher != null) {
+ return schemaDispatcher.dropSchema(schemaIdent(catalog.name(),
schemaName), cascade);
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof SupportsSchemas) {
+ return ((SupportsSchemas)
ops).dropSchema(NameIdentifier.of(schemaName), cascade);
+ }
+ }
+
+ return catalog.asSchemas().dropSchema(schemaName, cascade);
+ }
+
+ TableCatalog asTableCatalog(Catalog catalog) {
+ TableDispatcher tableDispatcher = currentTableDispatcher();
+ if (tableDispatcher != null) {
+ return new InternalTableCatalogAdapter(catalog.name(), tableDispatcher);
+ }
+
+ if (catalog instanceof BaseCatalog) {
+ CatalogOperations ops = ((BaseCatalog<?>) catalog).ops();
+ if (ops instanceof TableCatalog) {
+ return (TableCatalog) ops;
+ }
+ }
+
+ return catalog.asTableCatalog();
+ }
+
+ private NameIdentifier schemaIdent(String catalogName, String schemaName) {
+ return NameIdentifierUtil.ofSchema(metalakeName, catalogName, schemaName);
+ }
+
+ private NameIdentifier tableIdent(String catalogName, NameIdentifier ident) {
+ return NameIdentifierUtil.ofTable(
+ metalakeName, catalogName, ident.namespace().level(0), ident.name());
+ }
+
+ private Namespace tableNamespace(String catalogName, Namespace namespace) {
+ return Namespace.of(metalakeName, catalogName, namespace.level(0));
+ }
+
+ private SchemaDispatcher currentSchemaDispatcher() {
+ if (!config().isAuxMode()) {
+ return null;
+ }
+
+ return GravitinoEnv.getInstance().schemaDispatcher();
+ }
+
+ private TableDispatcher currentTableDispatcher() {
+ if (!config().isAuxMode()) {
+ return null;
+ }
+
+ return GravitinoEnv.getInstance().tableDispatcher();
+ }
+
+ @VisibleForTesting
+ CatalogOperator createCatalogOperator(String metalakeName) {
+ return config().isAuxMode()
+ ? new InternalCatalogOperator(metalakeName)
+ : new HttpCatalogOperator(
+ config().get(NAMESPACE_BACKEND_URI), metalakeName, config(),
extractClientProperties());
+ }
+
+ @VisibleForTesting
+ void setCatalogOperator(CatalogOperator catalogOperator) {
+ this.catalogOperator = catalogOperator;
+ }
+
+ private Map<String, String> extractClientProperties() {
+ Map<String, String> clientProperties = new HashMap<>();
+ config()
+ .getAllConfig()
+ .forEach(
+ (key, value) -> {
+ if (key.startsWith("gravitino.client.")) {
+ clientProperties.put(key, value);
+ LOG.info("Applying client config: {} = {}", key, value);
+ }
+ });
+ return clientProperties;
+ }
Review Comment:
Logging full configuration values at INFO can leak sensitive data if any
`gravitino.client.*` properties include credentials/tokens. Mask values (e.g.,
for keys containing `password`, `secret`, `token`, `credential`) and/or log
only keys (or downgrade to DEBUG) to prevent accidental secret exposure.
##########
dev/charts/gravitino-lance-rest-server/resources/gravitino-lance-rest-server.conf:
##########
@@ -39,9 +39,9 @@ gravitino.lance-rest.namespace-backend = {{
.Values.lanceRest.namespaceBackend |
gravitino.lance-rest.gravitino-uri = {{ .Values.lanceRest.gravitinoUri |
default "http://localhost:8090"; }}
# The metalake name used for Lance REST service gravitino namespace backend,
please create the metalake before using it, and configure the metalake name
here.
# Please set .Values.lanceRest.gravitinoMetalake to the name of a pre-created
metalake before deploying this chart.
-{{- if eq (.Values.lanceRest.namespaceBackend | default "gravitino")
"gravitino" }}
+{{ if eq (.Values.lanceRest.namespaceBackend | default "gravitino")
"gravitino" }}
gravitino.lance-rest.gravitino-metalake = {{ required
"lanceRest.gravitinoMetalake must be set when namespaceBackend is gravitino"
.Values.lanceRest.gravitinoMetalake }}
-{{- end }}
+{{ end }}
Review Comment:
The change from `{{- ... }}` / `{{- end }}` to `{{ ... }}` / `{{ end }}`
stops Helm whitespace trimming and can introduce extra blank lines/indentation
into the rendered config. Prefer keeping the trim markers (`{{- if ... }}` and
`{{- end }}`) to produce deterministic output and avoid introducing unintended
whitespace in config files.
##########
docs/lance-rest-service.md:
##########
@@ -155,16 +154,17 @@ To run Lance REST service independently without Gravitino
server (You need to st
Configure the service by editing
`{GRAVITINO_HOME}/conf/gravitino-lance-rest-server.conf` or passing
command-line arguments:
-| Configuration Property | Description |
Default Value | Required | Since Version |
-|-------------------------------------------|-----------------------------|-----------------------|----------|---------------|
-| `gravitino.lance-rest.namespace-backend` | Namespace metadata backend |
gravitino | Yes | 1.1.0 |
-| `gravitino.lance-rest.gravitino-uri` | Gravitino server URI |
http://localhost:8090 | Yes | 1.1.0 |
-| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name |
(none) | Yes | 1.1.0 |
-| `gravitino.lance-rest.httpPort` | Service port number |
9101 | No | 1.1.0 |
-| `gravitino.lance-rest.host` | Service hostname |
0.0.0.0 | No | 1.1.0 |
+| Configuration Property | Description
| Default Value | Required |
Since Version |
+|------------------------------------------------|---------------------------------------------------------------|---------------------------|----------|---------------|
+| `gravitino.lance-rest.namespace-backend` | Namespace metadata backend
| gravitino | Yes |
1.1.0 |
+| `gravitino.lance-rest.gravitino-uri` | Gravitino server URI
| http://localhost:8090 | Yes |
1.1.0 |
+| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name
| (none) | Yes |
1.1.0 |
+| `gravitino.lance-rest.httpPort` | Service port number
| 9101 | No |
1.1.0 |
+| `gravitino.lance-rest.host` | Service hostname
| 0.0.0.0 | No |
1.1.0 |
Review Comment:
The PR description states new standalone auth properties were added (e.g.,
`gravitino.lance-rest.gravitino-auth-type`, simple user, OAuth2 settings), but
this standalone configuration table doesn’t document them. Add the new
properties (and any related env var mappings, if supported) so users can
discover and correctly configure standalone authentication.
--
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]
