fix: reuse and close executor namespace clients by hfutatzhanghb · Pull Request #804 · lance-format/lance-spark

hfutatzhanghb · 2026-09-03T13:37:33Z

Summary

recreate the executor namespace lifecycle fix on top of the latest main
create the executor-side namespace client once per LanceColumnarPartitionReader instead of once per fragment
reuse the task-owned namespace across every fragment in the input partition
close AutoCloseable namespace implementations after fragment resources on normal completion and scan/open failures
give Arrow C stream exports and pushed COUNT(*) scans explicit namespace lifecycle owners
add regression coverage for columnar reuse, Arrow/count cleanup, failure cleanup, and the disabled-refresh path

This PR replaces #728 and uses a new branch based on current main.

Root cause

With executor_credential_refresh=true, LanceFragmentScanner.create() called
LanceRuntime.getOrCreateNamespace() before every fragment. Despite its name, that method
always creates a new LanceNamespace connection. The fragment scanner owned and closed its
native scanner and dataset, but nobody owned or closed the namespace client.

Hive2 and Hive3 namespace implementations are Closeable and own HMS client pools. As a
result, a Spark task scanning multiple fragments repeatedly created HMS-backed namespace
clients and leaked their connections.

Fix

LanceColumnarPartitionReader now owns executor namespace initialization and cleanup because
its lifecycle matches one Spark task. Initialization remains lazy and preserves the existing
executor_credential_refresh and useNamespaceOnWorkers gates. The namespace stays attached
to the task-local read options while fragments are scanned, then is cleared and closed after
the active fragment scanner.

The failure path preserves the original exception and attaches cleanup failures as suppressed
exceptions. Repeated reader close calls remain idempotent.

Validation

./mvnw spotless:check
./mvnw test -pl lance-spark-3.5_2.13 -DskipTests (main and test sources compile)
./mvnw test -pl lance-spark-3.5_2.13 -Dtest=org.lance.spark.internal.LanceFragmentScannerTest#testExecutorNamespaceOwnerClosesClient

…namespace-clients-main

hfutatzhanghb · 2026-09-04T05:52:42Z

Hi, Sir. @LuciferYang @geruh Could you please help review this PR when have free time? Thanks very much !!!

Assert that a post-acquire export failure still closes the task-owned namespace client on the construction catch path, not only on handle close.

geruh

Thanks for this @hfutatzhanghb! Just a few nits

geruh · 2026-09-04T15:43:08Z

+  }
+
+  @Test
+  public void testPartitionReaderReusesAndClosesExecutorNamespace() throws Exception {


These are constructing the LanceColumnarPartitionReader. Should they live in the LanceColumnarPartitionReaderTest file looks like they have a close() test

Thanks a lot. Very nice advice. I moved the three tests that construct LanceColumnarPartitionReader into LanceColumnarPartitionReaderTest alongside the existing close() coverage. I kept the direct ExecutorNamespace ownership test in place since it does not construct the partition reader and its RecordingNamespace fixture is also reused by LanceArrowStreamScannerTest.

geruh · 2026-09-04T15:45:53Z

-   * readOptions.setNamespace(...)} would fire — all observable here. The bogus dataset URI lets the
-   * outer {@code Utils.openDatasetBuilder().build()} call fail predictably, since the gate runs
-   * <i>before</i> the dataset is opened. No real Lance dataset is required.
+   * executor opens a namespace-backed table with the flag disabled, the partition reader must


nit: nice drop on the test java doc. I think we can drop this whole comment in favor of the test since i can understan what we are doing from the method name and the two asserts which mention refresh=false must not connect.

wdyt?

Agreed. The method name and assertion messages already explain the expected behavior, so I removed the Javadoc.

geruh · 2026-09-04T15:48:51Z

    }
-    // Null-first so close() is idempotent (PartitionReader extends Closeable, whose contract
-    // requires it): a repeat call short-circuits rather than raising
-    // `ArrowArrayStream is already closed` from a second ArrowArrayStream.release().


I think we might want to keep the comment here since the null-first is what makes close() idempotent, and spark closable isn't the only reason. wdyt?

// Null-first so close() is idempotent. A repeat call must not raise ArrowArrayStream is already closed from a second release().

Agreed. I restored the concise null-first comment above the resource reset so the idempotence requirement remains explicit. Thanks very much!

lance-gatekeeper

✅ Gate recommendation: approve.

This revision keeps the namespace lifecycle behavior unchanged while relocating the columnar-reader lifecycle tests to their owning suite and documenting why null-first cleanup makes repeated close calls safe. Coverage for disabled refresh, per-task reuse, failure cleanup, and idempotent close is preserved.

hfutatzhanghb · 2026-09-08T02:15:22Z

Hi, @geruh . Could you please review this PR again when have free time? Thanks very much!

hfutatzhanghb · 2026-09-09T03:37:41Z

@geruh Thanks very much for reviewing and merging!

zhanghaobo@kanzhun.com added 3 commits September 3, 2026 21:33

fix: close executor namespace clients

c827851

style: format namespace lifecycle tests

165e3aa

docs: clarify recording namespace discovery

ca2e2b1

hfutatzhanghb mentioned this pull request Sep 3, 2026

fix: reuse and close executor namespace clients #728

Closed

github-actions Bot added the bug Something isn't working label Sep 3, 2026

This comment was marked as outdated.

Sign in to view

lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 3, 2026

zhanghaobo@kanzhun.com added 2 commits September 4, 2026 09:47

Merge remote-tracking branch 'origin/main' into codex/close-executor-…

6524345

…namespace-clients-main

fix: close namespaces across executor scan paths

703bafb

lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 4, 2026

This comment was marked as outdated.

Sign in to view

lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 4, 2026

test: close executor namespace on Arrow export failure

4fad5af

Assert that a post-acquire export failure still closes the task-owned namespace client on the construction catch path, not only on handle close.

lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 4, 2026

This comment was marked as outdated.

Sign in to view

lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 4, 2026

geruh self-requested a review September 4, 2026 15:07

geruh reviewed Sep 4, 2026

View reviewed changes

test: address partition reader review nits

5b7e145

lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 7, 2026

lance-gatekeeper Bot reviewed Sep 7, 2026

View reviewed changes

lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. and removed K-approved Latest Gatekeeper recommendation permits acceptance. labels Sep 7, 2026

geruh approved these changes Sep 8, 2026

View reviewed changes

geruh merged commit 6765dcc into lance-format:main Sep 8, 2026
22 checks passed

hfutatzhanghb deleted the codex/close-executor-namespace-clients-main branch September 10, 2026 03:53

FANNG1 mentioned this pull request Sep 14, 2026

feat: distributed VECTOR_SEARCH over Gravitino Lance REST + S3 FANNG1/lance-spark#1

Open

FANNG1 mentioned this pull request Sep 24, 2026

feat: distributed VECTOR_SEARCH on Lance 12 #860

Open

Conversation

hfutatzhanghb commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Fix

Validation

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

hfutatzhanghb commented Sep 4, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

geruh left a comment

Choose a reason for hiding this comment

Uh oh!

geruh Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

hfutatzhanghb Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

geruh Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

hfutatzhanghb Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

geruh Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

hfutatzhanghb Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

lance-gatekeeper Bot left a comment

Choose a reason for hiding this comment

Uh oh!

hfutatzhanghb commented Sep 8, 2026

Uh oh!

Uh oh!

hfutatzhanghb commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hfutatzhanghb commented Sep 3, 2026 •

edited

Loading