Copilot commented on code in PR #11060:
URL: https://github.com/apache/gravitino/pull/11060#discussion_r3271762461
##########
core/build.gradle.kts:
##########
@@ -54,6 +54,13 @@ dependencies {
exclude(group = "com.fasterxml.jackson.jaxrs", module =
"jackson-jaxrs-json-provider") // using gravitino's version
exclude(group = "org.apache.httpcomponents.client5", module = "*") //
provided by gravitino
exclude(group = "org.lance", module = "lance-namespace-core") // This is
unnecessary in the core module
+ // Same rationale as lance-namespace-core: lance-core 4.0.1 declares
+ // lance-namespace-apache-client as a transitive, but core never calls
into it.
+ // Leaving it on the main classpath shadows the lance-rest aux service's
own
+ // lance-namespace-apache-client (loaded via lance-rest-server/libs/), and
+ // because the aux classloader is parent-first, the older transitive wins
+ // on request deserialization (e.g. dropping fields like `check_declared`).
+ exclude(group = "org.lance", module = "lance-namespace-apache-client")
Review Comment:
This comment references `lance-core 4.0.1`, but this PR upgrades
`lance-core` to 6.0.0. Updating the version mentioned here will keep the
rationale accurate for future maintainers.
##########
core/src/main/java/org/apache/gravitino/stats/storage/LancePartitionStatisticStorage.java:
##########
@@ -203,11 +204,12 @@ public LancePartitionStatisticStorage(Map<String, String>
properties) {
Caffeine.newBuilder()
.maximumSize(datasetCacheSize)
.scheduler(Scheduler.forScheduledExecutorService(this.scheduler))
- .evictionListener(
- (RemovalListener<Long, Dataset>)
+ .executor(Runnable::run)
+ .removalListener(
+ (RemovalListener<Long, DatasetHolder>)
(key, value, cause) -> {
if (value != null) {
- value.close();
+ closeDatasetHolder(value);
}
})
Review Comment:
The cache is configured with `.executor(Runnable::run)`, so
`DatasetHolder.close()` runs synchronously on the thread performing cache
mutations/evictions (including `cache.put` replacements during normal
operations). Closing Lance `Dataset` objects can be JNI/IO-heavy and this can
introduce latency spikes on the stats read/write path. Consider keeping
Caffeine’s default executor for removal notifications and explicitly closing
cached holders in `close()` (or otherwise ensuring allocator-close ordering) so
eviction/replacement cleanup doesn’t block request threads.
##########
clients/client-python/tests/integration/test_lance_ray.py:
##########
@@ -0,0 +1,335 @@
+# 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.
+
+import logging
+import os
+import shutil
+import tempfile
+import time
+import unittest
+from random import randint
+from typing import Optional
+
+import requests
+
+from gravitino import (
+ Catalog,
+ GravitinoAdminClient,
+ GravitinoClient,
+)
+from tests.integration.integration_test_env import IntegrationTestEnv
+
+logger = logging.getLogger(__name__)
+
+LANCE_REST_PORT = 9101
+LANCE_REST_BASE_URL = f"http://localhost:{LANCE_REST_PORT}/lance";
+
+# The Lance REST server runs as an auxiliary service inside the main
+# Gravitino process (gravitino.auxService.names = ...,lance-rest), so its
+# bind metalake is configured in the *main* gravitino.conf rather than the
+# standalone lance-rest conf file.
+MAIN_CONF_FILE = "conf/gravitino.conf"
+LANCE_REST_METALAKE_KEY = "gravitino.lance-rest.gravitino-metalake"
+
+
+def _missing_lance_ray_deps() -> Optional[str]:
+ missing = []
+ for mod in ("ray", "lance_ray", "lance_namespace"):
+ try:
+ __import__(mod)
+ except ImportError:
+ missing.append(mod)
+ return ", ".join(missing) if missing else None
+
+
+# Compute once at module import time so the @skipIf condition and message
+# don't trigger two rounds of import attempts.
+_MISSING_LANCE_RAY_DEPS = _missing_lance_ray_deps()
+
+
[email protected](
+ _MISSING_LANCE_RAY_DEPS is not None,
+ f"lance-ray test deps not installed: {_MISSING_LANCE_RAY_DEPS}. "
+ "Install with: pip install -e .[lance] (or pip install ray lance-ray "
+ "lance-namespace). Requires the Gravitino server to expose a lance-rest "
+ "auxiliary service backed by lance-namespace-core >= 0.7.5.",
+)
+class TestLanceRayIntegration(IntegrationTestEnv):
+ """End-to-end test for the lance-ray Python client against a
Gravitino-backed
+ Lance REST namespace. Mirrors the ``ray.data`` -> ``write_lance`` ->
+ ``read_lance`` flow from the upstream lance-ray docs.
+ """
+
+ # Metalake name is fixed because the lance-rest aux service binds to a
+ # single metalake from gravitino.conf. The per-test table name still gets
+ # a random suffix to keep individual test methods isolated.
+ METALAKE_NAME: str = "lance_ray_test_metalake"
+ CATALOG_NAME: str = "lance_catalog"
+ SCHEMA_NAME: str = "schema"
+ TABLE_NAME: str = "lance_ray_tbl_" + str(randint(1, 100000))
+
+ gravitino_admin_client: Optional[GravitinoAdminClient] = None
+ gravitino_client: Optional[GravitinoClient] = None
+ temp_dir: Optional[str] = None
+ main_conf_path: Optional[str] = None
+ appended_lance_rest_conf: bool = False
+
+ @classmethod
+ def setUpClass(cls):
+ super().setUpClass()
+
+ gravitino_home = os.environ.get("GRAVITINO_HOME")
+ if not gravitino_home:
+ raise RuntimeError(
+ "GRAVITINO_HOME must be set to the distribution package
directory"
+ )
Review Comment:
`setUpClass` raises a `RuntimeError` when `GRAVITINO_HOME` is unset. Other
Python integration tests call `IntegrationTestEnv._get_gravitino_home()` (which
exits/skips cleanly) before mutating conf or restarting the server. To avoid
failing the entire integration suite in environments where
`START_EXTERNAL_GRAVITINO=true` but `GRAVITINO_HOME` isn’t exported, align this
with the existing pattern (e.g., call `_get_gravitino_home()` and skip/exit
gracefully).
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/utils/LancePropertiesUtils.java:
##########
@@ -68,4 +68,14 @@ public static Map<String, String> resolveLanceStorageOptions(
effectiveStorageOptions.putAll(getLanceStorageOptions(tableProperties));
return effectiveStorageOptions;
}
+
+ public static Map<String, String> toTableProperties(Map<String, String>
storageOptions) {
+ if (storageOptions == null) {
+ return Map.of();
+ }
+
+ return storageOptions.entrySet().stream()
+ .collect(
+ Collectors.toMap(e -> LANCE_STORAGE_OPTIONS_PREFIX + e.getKey(),
Map.Entry::getValue));
+ }
Review Comment:
`toTableProperties` is a new `public` method but lacks Javadoc, while the
other public methods in this utility class are documented. This is likely to
fail the project’s Checkstyle/Javadoc requirements for new public APIs; please
add a brief Javadoc explaining the expected input (unprefixed storage options)
and output (properties with `lance.storage.` prefix) and any null/empty
behavior.
--
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]
