Remove cache manager, only have one config for cache size

Author: kris-gaudel

pyiceberg/manifest.py

@@ -50,7 +50,6 @@
     PrimitiveType,
     StringType,
     StructType,
-    strtobool,
 )
 from pyiceberg.utils.config import Config
 
@@ -893,79 +892,30 @@ def __hash__(self) -> int:
         return hash(self.manifest_path)
 
 
-class _ManifestCacheManager:
-    """Manages the manifest cache with lazy initialization from config."""
-
-    _DEFAULT_SIZE = 128
-
-    def __init__(self) -> None:
-        self._cache: LRUCache[str, ManifestFile] | None = None
-        self._initialized = False
-        self._lock = threading.RLock()
-
-    def get_cache(self) -> LRUCache[str, ManifestFile] | None:
-        """Return the cache if enabled, else None. Initializes from config on first call."""
-        with self._lock:
-            if self._initialized:
-                return self._cache
-
-            config = Config().config
-
-            # Extract nested config
-            manifest_val = config.get("manifest")
-            manifest_config: dict[str, Any] = manifest_val if isinstance(manifest_val, dict) else {}
-            cache_val = manifest_config.get("cache")
-            cache_config: dict[str, Any] = cache_val if isinstance(cache_val, dict) else {}
-
-            # Parse and validate enabled flag
-            enabled_raw = cache_config.get("enabled")
-            enabled = True
-            if enabled_raw is not None:
-                try:
-                    enabled = bool(strtobool(str(enabled_raw)))
-                except (ValueError, AttributeError) as err:
-                    raise ValueError(
-                        f"manifest.cache.enabled should be a boolean or left unset. Current value: {enabled_raw}"
-                    ) from err
-
-            # Parse and validate cache size
-            size_raw = cache_config.get("size")
-            size = self._DEFAULT_SIZE
-            if size_raw is not None:
-                try:
-                    size = int(str(size_raw))
-                except (ValueError, TypeError) as err:
-                    raise ValueError(
-                        f"manifest.cache.size should be a positive integer or left unset. Current value: {size_raw}"
-                    ) from err
-                if size < 1:
-                    raise ValueError(f"manifest.cache.size must be >= 1. Current value: {size}")
-
-            if enabled:
-                self._cache = LRUCache(maxsize=size)
-            self._initialized = True
-            return self._cache
-
-    def clear(self) -> None:
-        """Clear the cache contents. No-op if cache is disabled."""
-        cache = self.get_cache()
-        if cache is not None:
-            with self._lock:
-                cache.clear()
-
-
-# Module-level cache manager instance
-_manifest_cache_manager = _ManifestCacheManager()
-
-
-def _get_manifest_cache() -> LRUCache[str, ManifestFile] | None:
-    """Return the manifest cache if enabled, else None. Initializes from config on first call."""
-    return _manifest_cache_manager.get_cache()
+_DEFAULT_MANIFEST_CACHE_SIZE = 128
+_manifest_cache_lock = threading.RLock()
+
+
+def _init_manifest_cache() -> LRUCache[str, ManifestFile] | None:
+    """Initialize the manifest cache from config."""
+    manifest_cache_size = Config().get_int("manifest-cache-size")
+    if manifest_cache_size is None:
+        manifest_cache_size = _DEFAULT_MANIFEST_CACHE_SIZE
+    if manifest_cache_size < 0:
+        raise ValueError(f"manifest-cache-size must be >= 0. Current value: {manifest_cache_size}")
+    if manifest_cache_size == 0:
+        return None
+    return LRUCache(maxsize=manifest_cache_size)
+
+
+_manifest_cache = _init_manifest_cache()
 
 
 def clear_manifest_cache() -> None:
     """Clear the manifest cache. No-op if cache is disabled."""
-    _manifest_cache_manager.clear()
+    if _manifest_cache is not None:
+        with _manifest_cache_lock:
+            _manifest_cache.clear()
 
 
 def _manifests(io: FileIO, manifest_list: str) -> tuple[ManifestFile, ...]:
@@ -995,12 +945,12 @@ def _manifests(io: FileIO, manifest_list: str) -> tuple[ManifestFile, ...]:
     file = io.new_input(manifest_list)
     manifest_files = list(read_manifest_list(file))
 
-    cache = _get_manifest_cache()
-    if cache is None:
+    if _manifest_cache is None:
         return tuple(manifest_files)
 
     result = []
-    with _manifest_cache_manager._lock:
+    with _manifest_cache_lock:
+        cache = _manifest_cache
         for manifest_file in manifest_files:
             manifest_path = manifest_file.manifest_path
             if manifest_path in cache:

tests/benchmark/test_memory_benchmark.py

@@ -32,8 +32,9 @@
 import pyarrow as pa
 import pytest
 
+from pyiceberg import manifest as manifest_module
 from pyiceberg.catalog.memory import InMemoryCatalog
-from pyiceberg.manifest import _get_manifest_cache, clear_manifest_cache
+from pyiceberg.manifest import clear_manifest_cache
 
 
 def generate_test_dataframe() -> pa.Table:
@@ -95,7 +96,7 @@ def test_manifest_cache_memory_growth(memory_catalog: InMemoryCatalog) -> None:
         # Sample memory at intervals
         if (i + 1) % 10 == 0:
             current, _ = tracemalloc.get_traced_memory()
-            cache = _get_manifest_cache()
+            cache = manifest_module._manifest_cache
             cache_size = len(cache) if cache is not None else 0
 
             memory_samples.append((i + 1, current, cache_size))
@@ -151,7 +152,7 @@ def test_memory_after_gc_with_cache_cleared(memory_catalog: InMemoryCatalog) ->
 
     gc.collect()
     before_clear_memory, _ = tracemalloc.get_traced_memory()
-    cache = _get_manifest_cache()
+    cache = manifest_module._manifest_cache
     cache_size_before = len(cache) if cache is not None else 0
     print(f"  Memory before clear: {before_clear_memory / 1024:.1f} KB")
     print(f"  Cache size: {cache_size_before}")
@@ -193,7 +194,6 @@ def test_manifest_cache_deduplication_efficiency() -> None:
         FileFormat,
         ManifestEntry,
         ManifestEntryStatus,
-        _get_manifest_cache,
         _manifests,
         clear_manifest_cache,
         write_manifest,
@@ -269,7 +269,7 @@ def test_manifest_cache_deduplication_efficiency() -> None:
             _manifests(io, list_path)
 
         # Analyze cache efficiency
-        cache = _get_manifest_cache()
+        cache = manifest_module._manifest_cache
         cache_entries = len(cache) if cache is not None else 0
         # List i contains manifests 0..i, so only the first num_lists manifests are actually used
         manifests_actually_used = num_lists

tests/utils/test_manifest.py

@@ -52,9 +52,9 @@
 
 @pytest.fixture(autouse=True)
 def reset_global_manifests_cache() -> None:
-    # Reset cache state before each test so config is re-read
-    manifest_module._manifest_cache_manager._cache = None
-    manifest_module._manifest_cache_manager._initialized = False
+    with manifest_module._manifest_cache_lock:
+        manifest_module._manifest_cache = manifest_module._init_manifest_cache()
+    clear_manifest_cache()
 
 
 def _verify_metadata_with_fastavro(avro_file: str, expected_metadata: dict[str, str]) -> None:
@@ -810,7 +810,7 @@ def test_manifest_cache_deduplicates_manifest_files() -> None:
 
         # Verify cache size - should only have 3 unique ManifestFile objects
         # instead of 1 + 2 + 3 = 6 objects as with the old approach
-        cache = _get_manifest_cache()
+        cache = manifest_module._manifest_cache
         assert cache is not None, "Manifest cache should be enabled for this test"
         assert len(cache) == 3, f"Cache should contain exactly 3 unique ManifestFile objects, but has {len(cache)}"
 
@@ -885,7 +885,7 @@ def test_manifest_cache_efficiency_with_many_overlapping_lists() -> None:
         # With the new approach, we should have exactly N objects
 
         # Verify cache has exactly N unique entries
-        cache = _get_manifest_cache()
+        cache = manifest_module._manifest_cache
         assert cache is not None, "Manifest cache should be enabled for this test"
         assert len(cache) == num_manifests, (
             f"Cache should contain exactly {num_manifests} ManifestFile objects, "
@@ -1031,15 +1031,15 @@ def test_clear_manifest_cache() -> None:
         _manifests(io, list_path)
 
         # Verify cache has entries
-        cache = _get_manifest_cache()
+        cache = manifest_module._manifest_cache
         assert cache is not None, "Cache should be enabled"
         assert len(cache) > 0, "Cache should have entries after reading manifests"
 
         # Clear the cache
         clear_manifest_cache()
 
         # Verify cache is empty but still enabled
-        cache_after = _get_manifest_cache()
+        cache_after = manifest_module._manifest_cache
         assert cache_after is not None, "Cache should still be enabled after clear"
         assert len(cache_after) == 0, "Cache should be empty after clear"
 
@@ -1048,22 +1048,19 @@ def test_clear_manifest_cache() -> None:
     "env_vars,expected_enabled,expected_size",
     [
         ({}, True, 128),  # defaults
-        ({"PYICEBERG_MANIFEST__CACHE__ENABLED": "true"}, True, 128),
-        ({"PYICEBERG_MANIFEST__CACHE__ENABLED": "false"}, False, 128),
-        ({"PYICEBERG_MANIFEST__CACHE__SIZE": "64"}, True, 64),
-        ({"PYICEBERG_MANIFEST__CACHE__SIZE": "256"}, True, 256),
-        ({"PYICEBERG_MANIFEST__CACHE__ENABLED": "false", "PYICEBERG_MANIFEST__CACHE__SIZE": "64"}, False, 64),
+        ({"PYICEBERG_MANIFEST_CACHE_SIZE": "64"}, True, 64),
+        ({"PYICEBERG_MANIFEST_CACHE_SIZE": "256"}, True, 256),
+        ({"PYICEBERG_MANIFEST_CACHE_SIZE": "0"}, False, 0),  # size=0 disables cache
     ],
 )
 def test_manifest_cache_config_valid_values(env_vars: dict[str, str], expected_enabled: bool, expected_size: int) -> None:
     """Test that valid config values are applied correctly."""
     import os
 
     with mock.patch.dict(os.environ, env_vars, clear=False):
-        # Reset cache state so config is re-read
-        manifest_module._manifest_cache_manager._cache = None
-        manifest_module._manifest_cache_manager._initialized = False
-        cache = _get_manifest_cache()
+        with manifest_module._manifest_cache_lock:
+            manifest_module._manifest_cache = manifest_module._init_manifest_cache()
+        cache = manifest_module._manifest_cache
 
         if expected_enabled:
             assert cache is not None, "Cache should be enabled"
@@ -1075,20 +1072,15 @@ def test_manifest_cache_config_valid_values(env_vars: dict[str, str], expected_e
 @pytest.mark.parametrize(
     "env_vars,expected_error_substring",
     [
-        ({"PYICEBERG_MANIFEST__CACHE__ENABLED": "maybe"}, "manifest.cache.enabled should be a boolean"),
-        ({"PYICEBERG_MANIFEST__CACHE__ENABLED": "invalid"}, "manifest.cache.enabled should be a boolean"),
-        ({"PYICEBERG_MANIFEST__CACHE__SIZE": "abc"}, "manifest.cache.size should be a positive integer"),
-        ({"PYICEBERG_MANIFEST__CACHE__SIZE": "0"}, "manifest.cache.size must be >= 1"),
-        ({"PYICEBERG_MANIFEST__CACHE__SIZE": "-5"}, "manifest.cache.size must be >= 1"),
+        ({"PYICEBERG_MANIFEST_CACHE_SIZE": "abc"}, "manifest-cache-size should be an integer"),
+        ({"PYICEBERG_MANIFEST_CACHE_SIZE": "-5"}, "manifest-cache-size must be >= 0"),
     ],
 )
 def test_manifest_cache_config_invalid_values(env_vars: dict[str, str], expected_error_substring: str) -> None:
     """Test that invalid config values raise ValueError with appropriate message."""
     import os
 
     with mock.patch.dict(os.environ, env_vars, clear=False):
-        # Reset cache state so config is re-read
-        manifest_module._manifest_cache_manager._cache = None
-        manifest_module._manifest_cache_manager._initialized = False
         with pytest.raises(ValueError, match=expected_error_substring):
-            _get_manifest_cache()
+            with manifest_module._manifest_cache_lock:
+                manifest_module._manifest_cache = manifest_module._init_manifest_cache()