Conversation
58d5737 to
1b8c742
Compare
778b532 to
8e0ffeb
Compare
1bc7f4f to
2b0c8bb
Compare
8263c73 to
04d84c3
Compare
f7db5d0 to
5cef69a
Compare
5cef69a to
2b7ec5f
Compare
📝 WalkthroughWalkthroughThis update implements several changes related to hardware counter handling and telemetry measurement. A new module, Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/consensus_tests/test_hw_measurement.py (3)
27-31: Consider using a more efficient variable name for the unused parameter.The loop control variable
peer_infois not used within the loop body.- for peer_id, peer_info in collection_info['peers'].items(): + for peer_id, _ in collection_info['peers'].items():🧰 Tools
🪛 Ruff (0.8.2)
27-27: Loop control variable
peer_infonot used within loop bodyRename unused
peer_infoto_peer_info(B007)
45-45: Improve readability with consistent spacing.There's inconsistent spacing around the comma in the
assert_with_upper_bound_errorcall.- assert_with_upper_bound_error(hw['vector_io_write'], 1000 * 4 * 4,upper_bound_error_percent=0.2) # 1k vectors of dim 4 where each dim is 4 bytes + assert_with_upper_bound_error(hw['vector_io_write'], 1000 * 4 * 4, upper_bound_error_percent=0.2) # 1k vectors of dim 4 where each dim is 4 bytes
165-165: Improve assertion failure handling.Directly using
assert Falsecan be problematic when running Python with optimizations enabled (python -O), as assertion statements are removed.- assert False, f"Assertion {inp} < {upper_bound} (upperbound) failed. Allowed error = {upper_bound_error_percent}" + raise AssertionError(f"Assertion {inp} < {upper_bound} (upperbound) failed. Allowed error = {upper_bound_error_percent}")🧰 Tools
🪛 Ruff (0.8.2)
165-165: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/collection/src/collection/sharding_keys.rs(1 hunks)lib/common/common/src/counter/mod.rs(1 hunks)lib/common/common/src/counter/referenced_counter.rs(1 hunks)lib/gridstore/benches/bustle_bench/payload_storage.rs(2 hunks)lib/gridstore/benches/random_data_bench.rs(1 hunks)lib/gridstore/benches/real_data_bench.rs(2 hunks)lib/gridstore/src/gridstore.rs(19 hunks)lib/segment/src/payload_storage/mmap_payload_storage.rs(4 hunks)lib/segment/src/payload_storage/simple_payload_storage.rs(1 hunks)lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs(3 hunks)tests/consensus_tests/fixtures.py(2 hunks)tests/consensus_tests/test_hw_measurement.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/consensus_tests/test_hw_measurement.py
19-19: start_cluster may be undefined, or defined from star imports
(F405)
23-23: wait_collection_exists_and_active_on_all_peers may be undefined, or defined from star imports
(F405)
25-25: get_cluster_info may be undefined, or defined from star imports
(F405)
27-27: Loop control variable peer_info not used within loop body
Rename unused peer_info to _peer_info
(B007)
34-34: create_shard_key may be undefined, or defined from star imports
(F405)
55-55: start_cluster may be undefined, or defined from star imports
(F405)
57-57: wait_collection_exists_and_active_on_all_peers may be undefined, or defined from star imports
(F405)
165-165: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: test-consistency
- GitHub Check: test (macos-latest)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: test-consensus
- GitHub Check: test
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test
🔇 Additional comments (42)
tests/consensus_tests/test_hw_measurement.py (4)
18-52: Good addition of a test for the no-local-shard scenario.This test effectively verifies that hardware measurements are correctly captured by the targeted node when there is no local shard. The test demonstrates that the non-leader node captures telemetry data while the leader remains unaffected.
🧰 Tools
🪛 Ruff (0.8.2)
19-19:
start_clustermay be undefined, or defined from star imports(F405)
23-23:
wait_collection_exists_and_active_on_all_peersmay be undefined, or defined from star imports(F405)
25-25:
get_cluster_infomay be undefined, or defined from star imports(F405)
27-27: Loop control variable
peer_infonot used within loop bodyRename unused
peer_infoto_peer_info(B007)
34-34:
create_shard_keymay be undefined, or defined from star imports(F405)
56-56: Consistent parameter handling for sparse vectors across test functions.The update to include
sparse_vectors=Falseincreate_collectioncalls is consistent with the changes in fixtures.py. This ensures that the tests operate with the intended vector configuration.Also applies to: 123-123, 132-132
58-58: Consistent parameter handling for sparse vectors in point operations.The update to include
with_sparse_vector=Falseinupsert_random_pointscalls aligns with the requirement to test without sparse vectors. This maintains consistency with the collection configuration.Also applies to: 68-68, 132-132
164-165: Clarified assertion message improves test error diagnosis.The updated assertion message more explicitly states the failure condition, which helps with debugging test failures.
🧰 Tools
🪛 Ruff (0.8.2)
165-165: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
tests/consensus_tests/fixtures.py (3)
143-143: Good addition of sparse_vectors parameter with a default value.Adding the
sparse_vectorsparameter with a default ofTruemaintains backward compatibility while enabling more specific test configurations.
145-159: Well-structured payload construction with conditional logic.The refactored payload construction using a dictionary variable improves code readability and maintainability. The conditional inclusion of sparse vectors is cleanly implemented.
235-239: Improved error handling in get_telemetry_hw_info.The function now properly checks if the collection exists in the hardware data before accessing it, which prevents potential errors when the collection is not found.
lib/common/common/src/counter/mod.rs (1)
5-5: Module addition looks good.Adding the
referenced_countermodule is a clean way to introduce the new hardware counter reference functionality. This aligns with the PR objective of improving measurement and testing processes.lib/collection/src/collection/sharding_keys.rs (1)
64-64: Comment clarification is helpful.The updated comment accurately reflects that this is an internal operation that doesn't require measurement, which helps clarify the intention behind using a disposable hardware counter.
lib/gridstore/benches/real_data_bench.rs (2)
16-16: Good addition of hardware counter reference.Creating a reference to the payload IO write counter using the new
ref_payload_io_write_counter()method is consistent with the broader changes to how hardware counters are handled across the codebase.
27-27: Using the reference counter is consistent.Updating the
put_valuecall to use the new hardware counter reference maintains consistency with the API changes elsewhere in the codebase.lib/segment/src/payload_storage/simple_payload_storage.rs (1)
60-60: Counter type change is appropriate.Changing to
payload_io_write_counter()more accurately represents the type of operation being performed - writing payload data to storage. This makes the telemetry data more precise and meaningful.lib/gridstore/benches/bustle_bench/payload_storage.rs (2)
38-44: LGTM: Refined I/O measurement for payload insertionThe change to use
ref_payload_io_write_counter()instead of passing theHardwareCounterCelldirectly provides more specific measurement of payload I/O write operations.
52-57: LGTM: Consistent usage of I/O counter referencesThe update method now consistently uses the specific payload I/O write counter reference, aligning with the insertion method and providing more granular measurement.
lib/gridstore/benches/random_data_bench.rs (2)
13-13: LGTM: Improved counter reference handlingCreating a specific reference to the payload I/O write counter improves the clarity and specificity of what's being measured.
18-18: LGTM: Consistent usage of the payload counter referenceThe bench iteration now uses the dedicated counter reference instead of the general hardware counter, providing more accurate measurement of payload I/O write operations.
lib/segment/src/vector_storage/sparse/mmap_sparse_vector_storage.rs (3)
154-159: LGTM: Improved specificity for vector I/O measurementsUsing
ref_vector_io_write_counter()makes it clear that this operation specifically measures vector I/O write operations, providing more detailed telemetry.
284-284: LGTM: Clarified measurement policyThe comment now explicitly states that deletions are not measured, which provides valuable documentation for anyone working with or analyzing the telemetry data.
236-236: LGTM: Documentation of measurement policyComment clarifies that this function is only used for internal operations and doesn't need to be measured, which is helpful context for future developers.
lib/segment/src/payload_storage/mmap_payload_storage.rs (6)
72-74: LGTM: Consistent use of payload I/O counterChanged to use the specific payload I/O write counter reference, consistent with other modifications in the PR.
89-94: LGTM: Refactored counter reference in set methodThe first branch of the set method now properly uses the dedicated payload I/O write counter reference.
98-99: LGTM: Consistent counter usage in set methodThe second branch of the set method now uses the same payload I/O write counter reference approach, ensuring consistency across the codebase.
117-122: LGTM: Applied counter reference pattern to set_by_keyThe first branch of the set_by_key method follows the same pattern of using the specific counter reference.
128-133: LGTM: Consistent counter reference in set_by_keyThe second branch of the set_by_key method maintains consistency with the counter reference pattern.
162-167: LGTM: Applied counter reference pattern to delete methodThe delete method now also uses the payload I/O write counter reference, completing the consistent application of this pattern throughout the file.
lib/common/common/src/counter/referenced_counter.rs (1)
1-39: Well-designed and clearly documented abstraction!This is a good implementation of a wrapper type that restricts access to only specific metrics within a
HardwareCounterCell. The use ofDerefallows transparent usage at call sites, while hiding the constructor and only exposing factory methods enforces the correct pattern of usage.I particularly like:
- The clear documentation explaining the purpose and use cases
- Making the constructor private to control instantiation
- The use of
#[inline]for the factory methods to avoid any performance penaltylib/gridstore/src/gridstore.rs (16)
5-5: Added import for new counter typeThe import is correctly added to support the parameter type change in
put_value.
326-331: Improved parameter type specificity input_valueThe parameter type change from
&HardwareCounterCelltoHwMetricRefCountermakes the function signature more explicit about which metric is being tracked. This is a good change that helps with code clarity and correctness.
384-384: Simplified counter usageThe call to increment the counter is now more direct, which is a result of the more specific parameter type. This makes the code cleaner and less error-prone.
562-563: Updated test for new counter referencing patternThe test has been properly updated to use the new referencing pattern.
587-590: Updated test code to use the new counter referencing patternTest updated correctly to match the new API.
615-617: More descriptive variable naming in testThe use of
hw_counter_refmakes it clearer that this is a reference to a specific counter rather than the general hardware counter cell.
652-657: Test updated to use new counter referencing patternThe test has been correctly updated to use the new
ref_payload_io_write_counter()method.
683-685: Test updated to use new counter referencing patternThis test update is consistent with the pattern used in other tests.
717-720: Test updated to use new counter referencing patternAll tests have been consistently updated to match the new API.
740-742: Test updated to use new counter referencing patternAll instances of the
put_valuecall have been updated to use the new referencing pattern consistently.
819-820: Test updated to use new counter referencing patternThe complex test case "behave_like_hashmap" is also correctly updated.
900-901: Test updated to use new counter referencing patternThe "handle_huge_payload" test is also correctly updated.
942-945: Test updated to use new counter referencing patternThe "storage_persistence_basic" test is also correctly updated.
981-997: Test helper function updated for new counter referencing patternThe write_data helper function is correctly updated to use the new pattern.
1065-1085: Test updated to use new counter referencing patternBoth instances of
put_valuein this test section have been properly updated.
1121-1128: Final test updated for new counter referencing patternAll tests throughout the file have been consistently updated to the new pattern.
* HW measurement cleanup and more tests * Fix wrong measurements * Use new assertion function * Fix Payload Measurement bug. Fix tests
Depends on #5944