From a69ddffed8d2d92bd2a27507630edcdfd1148ded Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20Vis=C3=A9e?= Date: Mon, 13 Jan 2025 17:45:14 +0100 Subject: [PATCH] Enable mmap storage using blob store for payloads by default (#5783) * Enable mmap storage using blob store for payloads by default * adapt test for new mmap payload storage --------- Co-authored-by: Arnaud Gourlay --- lib/collection/src/config.rs | 12 +----------- lib/collection/src/operations/conversions.rs | 1 - .../src/content_manager/toc/create_collection.rs | 1 - lib/storage/src/types.rs | 3 --- lib/storage/tests/integration/alias_tests.rs | 1 - tests/consensus_tests/test_strict_mode.py | 11 +---------- 6 files changed, 2 insertions(+), 27 deletions(-) diff --git a/lib/collection/src/config.rs b/lib/collection/src/config.rs index fc3c73d0db..eb24759f81 100644 --- a/lib/collection/src/config.rs +++ b/lib/collection/src/config.rs @@ -105,10 +105,6 @@ pub struct CollectionParams { /// Default: true #[serde(default = "default_on_disk_payload")] pub on_disk_payload: bool, - /// Temporary setting to enable/disable the use of mmap for on-disk payload storage. - // TODO: remove this setting after integration is finished - #[serde(skip)] - pub on_disk_payload_uses_mmap: bool, /// Configuration of the sparse vector storage #[serde(default, skip_serializing_if = "Option::is_none")] #[validate(nested)] @@ -118,10 +114,7 @@ pub struct CollectionParams { impl CollectionParams { pub fn payload_storage_type(&self) -> PayloadStorageType { if self.on_disk_payload { - if self.on_disk_payload_uses_mmap { - return PayloadStorageType::Mmap; - } - PayloadStorageType::OnDisk + PayloadStorageType::Mmap } else { PayloadStorageType::InMemory } @@ -136,7 +129,6 @@ impl CollectionParams { write_consistency_factor: _, // May be changed read_fan_out_factor: _, // May be changed on_disk_payload: _, // May be changed - on_disk_payload_uses_mmap: _, // Temporary sparse_vectors, // Parameters may be changes, but not the structure } = other; @@ -187,7 +179,6 @@ impl Anonymize for CollectionParams { write_consistency_factor: self.write_consistency_factor, read_fan_out_factor: self.read_fan_out_factor, on_disk_payload: self.on_disk_payload, - on_disk_payload_uses_mmap: self.on_disk_payload_uses_mmap, sparse_vectors: self.sparse_vectors.anonymize(), } } @@ -273,7 +264,6 @@ impl CollectionParams { write_consistency_factor: default_write_consistency_factor(), read_fan_out_factor: None, on_disk_payload: default_on_disk_payload(), - on_disk_payload_uses_mmap: false, sparse_vectors: None, } } diff --git a/lib/collection/src/operations/conversions.rs b/lib/collection/src/operations/conversions.rs index 5a2175b911..0d0103d80f 100644 --- a/lib/collection/src/operations/conversions.rs +++ b/lib/collection/src/operations/conversions.rs @@ -1809,7 +1809,6 @@ impl TryFrom for CollectionConfig { .sharding_method .map(sharding_method_from_proto) .transpose()?, - on_disk_payload_uses_mmap: false, }, }, hnsw_config: match config.hnsw_config { diff --git a/lib/storage/src/content_manager/toc/create_collection.rs b/lib/storage/src/content_manager/toc/create_collection.rs index 47064a5c1c..1da9fa2328 100644 --- a/lib/storage/src/content_manager/toc/create_collection.rs +++ b/lib/storage/src/content_manager/toc/create_collection.rs @@ -141,7 +141,6 @@ impl TableOfContent { .ok_or_else(|| StorageError::bad_input("`shard_number` cannot be 0"))?, sharding_method, on_disk_payload: on_disk_payload.unwrap_or(self.storage_config.on_disk_payload), - on_disk_payload_uses_mmap: self.storage_config.on_disk_payload_uses_mmap, replication_factor: NonZeroU32::new(replication_factor).ok_or_else(|| { StorageError::BadInput { description: "`replication_factor` cannot be 0".to_string(), diff --git a/lib/storage/src/types.rs b/lib/storage/src/types.rs index 4a3076eaa8..a64159824d 100644 --- a/lib/storage/src/types.rs +++ b/lib/storage/src/types.rs @@ -66,9 +66,6 @@ pub struct StorageConfig { pub temp_path: Option, #[serde(default = "default_on_disk_payload")] pub on_disk_payload: bool, - // TODO: remove this field after integration is finished - #[serde(default)] - pub on_disk_payload_uses_mmap: bool, #[validate(nested)] pub optimizers: OptimizersConfig, #[validate(nested)] diff --git a/lib/storage/tests/integration/alias_tests.rs b/lib/storage/tests/integration/alias_tests.rs index e4a3b82bdf..6626b62f55 100644 --- a/lib/storage/tests/integration/alias_tests.rs +++ b/lib/storage/tests/integration/alias_tests.rs @@ -37,7 +37,6 @@ fn test_alias_operation() { snapshots_config: Default::default(), temp_path: None, on_disk_payload: false, - on_disk_payload_uses_mmap: false, optimizers: OptimizersConfig { deleted_threshold: 0.5, vacuum_min_vector_number: 100, diff --git a/tests/consensus_tests/test_strict_mode.py b/tests/consensus_tests/test_strict_mode.py index 71ce916cb5..de5ae1dcf6 100644 --- a/tests/consensus_tests/test_strict_mode.py +++ b/tests/consensus_tests/test_strict_mode.py @@ -118,8 +118,6 @@ def test_vector_storage_strict_mode_upsert_local_shard(tmp_path: pathlib.Path): def test_payload_strict_mode_upsert(tmp_path: pathlib.Path): - # TODO: Update this test when payload rocksDB storage has been replaced by mmap payload storage! - # Every required change for the mmap migration has been marked with a "TODO" peer_urls, peer_dirs, bootstrap_url = start_cluster(tmp_path, 4) strict_mode = { @@ -133,7 +131,6 @@ def test_payload_strict_mode_upsert(tmp_path: pathlib.Path): # Insert points into leader for i in range(10): upsert_random_points(peer_urls[0], 100, collection_name=COLLECTION_NAME, offset=i*100) - time.sleep(5) # TODO: Remove # Check that each node blocks new points now for peer_url in peer_urls: @@ -143,14 +140,11 @@ def test_payload_strict_mode_upsert(tmp_path: pathlib.Path): if not res.ok: assert "Max payload storage size" in res.json()['status']['error'] return - time.sleep(1) # TODO: Remove assert False, "Should have blocked upsert but didn't" def test_payload_strict_mode_upsert_no_local_shard(tmp_path: pathlib.Path): - # TODO: Update this test when payload rocksDB storage has been replaced by mmap payload storage! - # Every required change for the mmap migration has been marked with a "TODO" peer_urls, peer_dirs, bootstrap_url = start_cluster(tmp_path, N_PEERS) create_collection(peer_urls[0], collection=COLLECTION_NAME, shard_number=1, replication_factor=N_REPLICAS, sharding_method="custom") @@ -172,11 +166,10 @@ def test_payload_strict_mode_upsert_no_local_shard(tmp_path: pathlib.Path): for _ in range(32): point = {"id": 1, "payload": payload, "vector": random_dense_vector()} upsert_points(peer_urls[0], [point], collection_name=COLLECTION_NAME, shard_key="non_leader").raise_for_status() - time.sleep(1) # TODO: Remove set_strict_mode(peer_urls[0], COLLECTION_NAME, { "enabled": True, - "max_collection_payload_size_bytes": 15000, + "max_collection_payload_size_bytes": 10_000, }) wait_for_strict_mode_enabled(peer_urls[1], COLLECTION_NAME) @@ -184,7 +177,6 @@ def test_payload_strict_mode_upsert_no_local_shard(tmp_path: pathlib.Path): for _ in range(32): point = {"id": 2, "payload": payload, "vector": random_dense_vector()} upsert_points(peer_urls[0], [point], collection_name=COLLECTION_NAME, shard_key="non_leader").raise_for_status() - time.sleep(1) # TODO: Remove for _ in range(32): point = {"id": 3, "payload": payload, "vector": random_dense_vector()} @@ -193,7 +185,6 @@ def test_payload_strict_mode_upsert_no_local_shard(tmp_path: pathlib.Path): assert "Max payload storage size" in res.json()['status']['error'] assert not res.ok return - time.sleep(1) # TODO: Remove assert False, "Should have blocked upsert but didn't"