apache/gravitino

[Improvement] Fix dataset leak on cache replacement in LancePartitionStatisticStorage

Open

#10,326 opened on Mar 10, 2026

View on GitHub
 (2 comments) (0 reactions) (1 assignee)Java (887 forks)auto 404
good first issueimprovement

Repository metrics

Stars
 (3,058 stars)
PR merge metrics
 (PR metrics pending)

Description

What would you like to be improved?

When datasetCacheSize > 0, appendStatisticsImpl commits and then does cache.put(tableId, newDataset). Replacing an existing cache value does not trigger the current close path, because the cache is wired with evictionListener, while replacement is a removal event. The old Dataset can remain unclosed, causing leaked native/file handles during repeated statistics updates. Default config (datasetCacheSize=0) avoids this path, but cached deployments are affected.

How should we improve?

Ensure replaced cache entries are closed on any removal, not just eviction. Update cache wiring to close Dataset via a removal callback that runs for replacement/removal/eviction events. Keep invalidateAll() on shutdown so cached datasets are closed at storage close.

Here's a unit test to help:

  @Test
  public void testDatasetCacheReplacementClosesOldDataset() throws Exception {
    String location = Files.createTempDirectory("lance_stats_cache_replace").toString();

    Map<String, String> properties = Maps.newHashMap();
    properties.put("location", location);
    properties.put("datasetCacheSize", "10");

    LancePartitionStatisticStorage storage = new LancePartitionStatisticStorage(properties);
    try {
      Cache<Long, Dataset> cache = storage.getDatasetCache();
      Dataset oldDataset = mock(Dataset.class);
      Dataset newDataset = mock(Dataset.class);

      cache.put(1L, oldDataset);
      cache.put(1L, newDataset);

      verify(oldDataset, times(1)).close();
    } finally {
      FileUtils.deleteDirectory(new File(location));
      storage.close();
    }
  }

Contributor guide