Overall improvements #129
Overall improvements #129
Conversation
WalkthroughWorkspace and crate versions bumped to 0.2.1; BlockWriter APIs changed to accept Arc-wrapped blocks/batches; RocksDB and other dependencies updated; cache TTLs and mark_for_removal semantics changed to immediate removal; tests and CI adjusted for Arc usage and constrained test threads. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Trait as BlockWriter Trait
participant Impl as Impl/Service
participant Storage
Note over Caller,Trait: Old flow (by-ref)
Caller->>Trait: write_block(&Block)
Trait->>Impl: &Block (borrow)
Impl->>Impl: clone block for blocking task
Impl->>Storage: store(cloned Block)
Note over Caller,Storage: New flow (Arc)
Caller->>Caller: Arc::new(Block)
Caller->>Trait: write_block(Arc<Block>)
Trait->>Impl: Arc<Block> (shared)
Impl->>Impl: Arc::clone (cheap pointer copy)
Impl->>Storage: store(Arc reference)
rect rgb(230,245,255)
Note right of Impl: Batch APIs now accept Arc<Vec<Block>>\navoiding per-element cloning
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/rock-node-state-management-plugin/Cargo.toml (1)
26-26: Verify RocksDB 0.24 compatibility.As noted in the backfill-plugin review, ensure the RocksDB upgrade from 0.22 to 0.24 doesn't introduce breaking changes or require migration steps for existing state databases.
🧹 Nitpick comments (13)
crates/rock-node-observability-plugin/src/lib.rs (1)
345-362: Excellent test coverage for capability-dependent readiness!The test correctly validates that the readiness endpoint returns
NOT_READYbefore capability registration andREADYafter registeringProvidesBlockReader. The test logic is sound and comprehensive.Optional: Remove redundant import.
Line 347 imports
Statelocally, but it's already imported at the top of the file (line 4).#[tokio::test] async fn test_readiness_depends_on_block_reader_capability() { - use axum::extract::State; - let ctx = create_test_context(true);nginx.conf (1)
49-83: Consider refactoring duplicate gRPC location blocks.The three new location blocks have identical configuration to the existing
BlockStreamPublishServiceblock. This duplication makes the configuration harder to maintain.Please verify the 24-hour gRPC timeouts are appropriate for all services, especially for non-streaming endpoints like
BlockNodeServiceandBlockAccessService.Consider using a regex location or shared configuration snippet to reduce duplication:
+ # Shared gRPC configuration + map $uri $is_block_service { + "~^/org\.hiero\.block\.api\.(BlockStreamPublishService|BlockStreamSubscribeService|BlockNodeService|BlockAccessService)" 1; + default 0; + } + server { listen 6791; http2 on; access_log /var/log/nginx/access.log; error_log /var/log/nginx/error.log info; - location /org.hiero.block.api.BlockStreamPublishService { + location ~ ^/org\.hiero\.block\.api\.(BlockStreamPublishService|BlockStreamSubscribeService|BlockNodeService|BlockAccessService) { proxy_request_buffering off; grpc_read_timeout 24h; grpc_send_timeout 24h; grpc_pass grpc://rock_node_grpc; grpc_set_header X-Real-IP $remote_addr; grpc_set_header X-Forwarded-For $proxy_add_x_forwarded_for; grpc_set_header Host $host; } - location /org.hiero.block.api.BlockStreamSubscribeService { - proxy_request_buffering off; - grpc_read_timeout 24h; - grpc_send_timeout 24h; - - grpc_pass grpc://rock_node_grpc; - - grpc_set_header X-Real-IP $remote_addr; - grpc_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - grpc_set_header Host $host; - } - - location /org.hiero.block.api.BlockNodeService { - proxy_request_buffering off; - grpc_read_timeout 24h; - grpc_send_timeout 24h; - - grpc_pass grpc://rock_node_grpc; - - grpc_set_header X-Real-IP $remote_addr; - grpc_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - grpc_set_header Host $host; - } - - location /org.hiero.block.api.BlockAccessService { - proxy_request_buffering off; - grpc_read_timeout 24h; - grpc_send_timeout 24h; - - grpc_pass grpc://rock_node_grpc; - - grpc_set_header X-Real-IP $remote_addr; - grpc_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - grpc_set_header Host $host; - } - location / { return 404; }Alternatively, if different services require different configurations in the future, the current approach provides more flexibility.
review_comment_end -->crates/rock-node-state-management-plugin/src/plugin.rs (4)
54-65: Initialization logic correctly handles disabled stateThe initialization properly reads the configuration flag and conditionally skips internal setup when disabled, while still preserving the context for potential future use. The early return prevents unnecessary initialization of the
StateManagerand registration ofStateReaderProvider.For consistency with the disabled path, consider adding a log entry when the plugin is enabled:
self.enabled = context.config.plugins.state_management_service.enabled; if !self.enabled { info!("StateManagementPlugin is disabled via configuration; skipping initialization."); self.app_context = Some(context); return Ok(()); } + info!("StateManagementPlugin is enabled; proceeding with initialization."); self.init_internal(context)
126-135: Early exit for disabled plugin is correctThe disabled check properly prevents the plugin from starting its background task when disabled via configuration.
Minor optimization: the context is cloned even when the plugin is disabled. Consider moving the clone after the enabled check:
fn start_internal(&mut self) -> Result<()> { + if !self.enabled { + info!("StateManagementPlugin start skipped because it is disabled via configuration."); + return Ok(()); + } + let context = self .app_context .clone() .context("AppContext not initialized")?; - if !self.enabled { - info!("StateManagementPlugin start skipped because it is disabled via configuration."); - return Ok(()); - } - // Check if start_block_number is non-zero
231-251: Consider consolidating test context creation helpersA similar
create_test_context()helper exists intests/integration/src/fixtures/mock_plugins.rs. While the signatures differ slightly, consider extracting a shared test utility to reduce code duplication.You could either:
- Extend the existing helper in
tests/integration/src/fixtures/mock_plugins.rsto accept an optionalStateManagementServiceConfigparameter, or- Move this helper to a shared test utilities module (e.g., alongside
create_isolated_metricsincrates/rock-node-core/src/test_utils.rs) that both test files can import.
281-291: Test correctly verifies disabled plugin doesn't startThe test properly validates that calling
start()on a disabled plugin leaves it in a non-running state.Consider adding a complementary test case that verifies the plugin still initializes and starts correctly when
enabled=true. This would provide regression protection and demonstrate that the new feature doesn't break existing functionality.Example test structure:
#[tokio::test] async fn test_initialize_enabled_proceeds_normally() { let ctx = create_test_context(true); // Add required providers (DatabaseManagerProvider, BlockReaderProvider) // to ctx.service_providers for successful initialization let mut plugin = StateManagementPlugin::new(); plugin.initialize(ctx.clone()).unwrap(); assert!(plugin.enabled, "plugin should be enabled"); assert!(plugin.state_manager.is_some(), "state manager should be created"); // Verify StateReaderProvider is registered }Note: This test would require setting up the necessary providers for initialization to succeed.
charts/rock-node/templates/serviceaccount.yaml.tpl (1)
1-16: Consider usingnindentinstead ofindentfor cleaner YAML output.The template structure is correct, but Helm's
nindentfunction is preferred overindentas it ensures a newline is added before indentation, producing cleaner YAML output and avoiding potential formatting issues.Apply this diff:
{{- with .Values.serviceAccount.annotations }} annotations: -{{ toYaml . | indent 4 }} + {{- toYaml . | nindent 4 }} {{- end }}charts/rock-node/README.md (1)
15-15: Minor: Consider hyphenating compound adjective.The phrase "environment specific" should be hyphenated as "environment-specific" when used as a compound adjective before a noun.
Apply this diff:
-Override image repository, tag, and any environment specific configuration as +Override image repository, tag, and any environment-specific configuration ascharts/rock-node/Chart.yaml (1)
6-6: Consider using a specific version instead of "latest".The
appVersion: "latest"is convenient for development but can lead to unpredictable deployments in production environments. Consider using the actual application version (e.g., "0.2.1") to ensure reproducible deployments.charts/rock-node/values.yaml (1)
25-31: Consider the CPU limit-to-request ratio.The CPU limit (2000m) is 8× the request (250m), which can lead to throttling issues and creates a "Burstable" QoS class. For production workloads, consider:
- Using a smaller multiplier (1.5-2×) to reduce throttling risk
- Setting request equal to limit for "Guaranteed" QoS class
- Profiling actual usage to set appropriate values
crates/rock-node-core/src/test_utils.rs (1)
177-183: Consider storing Arc instead of cloning the underlying Block.The current implementation dereferences the Arc and clones the entire Block, which defeats the purpose of using Arc for shared ownership. Since this is a mock for testing, you could either:
- Store
Arc<Block>directly in the vector (changeblocksfield toArc<RwLock<Vec<Arc<Block>>>>)- Accept that this is test code and the clone is acceptable for test isolation
If you choose option 1, apply this diff:
pub struct MockBlockWriter { - blocks: Arc<std::sync::RwLock<Vec<rock_node_protobufs::com::hedera::hapi::block::stream::Block>>>, + blocks: Arc<std::sync::RwLock<Vec<Arc<rock_node_protobufs::com::hedera::hapi::block::stream::Block>>>>, }And update the implementation:
async fn write_block( &self, block: Arc<rock_node_protobufs::com::hedera::hapi::block::stream::Block>, ) -> anyhow::Result<()> { - self.blocks.write().unwrap().push((*block).clone()); + self.blocks.write().unwrap().push(block); Ok(()) }charts/rock-node/templates/deployment.yaml.tpl (1)
56-61: Simplify the extraEnv conditional logic.The current pattern renders an empty array
[]when extraEnv is not defined, which is unnecessary. Helm'swithdirective already handles empty/nil values gracefully.Apply this diff to simplify:
env: - {{- if .Values.extraEnv }} -{{ toYaml .Values.extraEnv | indent 12 }} - {{- else }} - [] - {{- end }} + {{- with .Values.extraEnv }} +{{ toYaml . | indent 12 }} + {{- end }}This will omit the entire
env:section when extraEnv is empty, which is cleaner and more idiomatic Helm.crates/rock-node-core/src/block_writer.rs (1)
72-79: Consider storing Arc to avoid cloning in test mock.Similar to the MockBlockWriter in test_utils.rs, this implementation dereferences the Arc and clones the entire Block (line 77), which defeats the purpose of using Arc. For production code, this pattern is correct (like in PersistenceService), but for a test mock, you could store Arc directly.
Apply this diff if you want to avoid cloning in tests:
#[derive(Debug)] struct MockBlockWriter { - blocks: RwLock<Vec<Block>>, + blocks: RwLock<Vec<Arc<Block>>>, should_fail: RwLock<bool>, }And update the write method:
async fn write_block(&self, block: Arc<Block>) -> Result<()> { if *self.should_fail.read().unwrap() { return Err(anyhow::anyhow!("Mock write failure")); } - self.blocks.write().unwrap().push((*block).clone()); + self.blocks.write().unwrap().push(block); Ok(()) }Note: This would require updating
get_written_blocks()to returnVec<Arc<Block>>or dereference when reading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
Cargo.toml(1 hunks)app/rock-node/Cargo.toml(1 hunks)charts/rock-node/Chart.yaml(1 hunks)charts/rock-node/README.md(1 hunks)charts/rock-node/templates/_helpers.tpl(1 hunks)charts/rock-node/templates/configmap.yaml.tpl(1 hunks)charts/rock-node/templates/deployment.yaml.tpl(1 hunks)charts/rock-node/templates/ingress.yaml.tpl(1 hunks)charts/rock-node/templates/pvc.yaml.tpl(1 hunks)charts/rock-node/templates/service.yaml.tpl(1 hunks)charts/rock-node/templates/serviceaccount.yaml.tpl(1 hunks)charts/rock-node/templates/servicemonitor.yaml.tpl(1 hunks)charts/rock-node/values.yaml(1 hunks)crates/rock-node-backfill-plugin/Cargo.toml(2 hunks)crates/rock-node-backfill-plugin/examples/backfill_continious_mode.rs(1 hunks)crates/rock-node-backfill-plugin/src/lib.rs(1 hunks)crates/rock-node-backfill-plugin/src/worker.rs(3 hunks)crates/rock-node-block-access-plugin/Cargo.toml(1 hunks)crates/rock-node-core/Cargo.toml(2 hunks)crates/rock-node-core/src/block_writer.rs(12 hunks)crates/rock-node-core/src/cache.rs(4 hunks)crates/rock-node-core/src/test_utils.rs(2 hunks)crates/rock-node-observability-plugin/Cargo.toml(1 hunks)crates/rock-node-observability-plugin/src/lib.rs(3 hunks)crates/rock-node-persistence-plugin/Cargo.toml(2 hunks)crates/rock-node-persistence-plugin/src/lib.rs(2 hunks)crates/rock-node-persistence-plugin/src/service.rs(6 hunks)crates/rock-node-protobufs/Cargo.toml(1 hunks)crates/rock-node-publish-plugin/Cargo.toml(1 hunks)crates/rock-node-query-plugin/Cargo.toml(1 hunks)crates/rock-node-server-status-plugin/Cargo.toml(1 hunks)crates/rock-node-state-management-plugin/Cargo.toml(2 hunks)crates/rock-node-state-management-plugin/src/plugin.rs(5 hunks)crates/rock-node-subscriber-plugin/Cargo.toml(1 hunks)crates/rock-node-verifier-plugin/Cargo.toml(1 hunks)nginx.conf(3 hunks)tests/e2e/Cargo.toml(2 hunks)tests/integration/Cargo.toml(1 hunks)tests/integration/src/tests/end_to_end_workflows.rs(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
crates/rock-node-backfill-plugin/src/worker.rs (3)
crates/rock-node-backfill-plugin/src/lib.rs (2)
write_block(147-152)write_block_batch(154-161)crates/rock-node-core/src/block_writer.rs (4)
write_block(13-13)write_block(73-79)write_block_batch(19-19)write_block_batch(81-87)crates/rock-node-core/src/test_utils.rs (2)
write_block(177-183)write_block_batch(185-191)
crates/rock-node-core/src/test_utils.rs (5)
crates/rock-node-backfill-plugin/examples/backfill_continious_mode.rs (1)
write_block_batch(61-67)crates/rock-node-backfill-plugin/src/lib.rs (2)
write_block_batch(154-161)new(27-32)crates/rock-node-backfill-plugin/src/worker.rs (6)
write_block_batch(749-756)new(190-226)new(619-621)new(670-676)new(720-725)new(843-853)crates/rock-node-core/src/block_writer.rs (4)
write_block_batch(19-19)write_block_batch(81-87)new(29-31)new(51-56)crates/rock-node-persistence-plugin/src/service.rs (2)
write_block_batch(200-242)new(27-43)
crates/rock-node-state-management-plugin/src/plugin.rs (3)
crates/rock-node-core/src/test_utils.rs (1)
create_isolated_metrics(55-58)tests/integration/src/fixtures/mock_plugins.rs (1)
create_test_context(103-120)crates/rock-node-core/src/config.rs (10)
default(23-31)default(58-63)default(76-83)default(93-95)default(115-126)default(141-149)default(165-167)default(177-179)default(189-191)default(231-239)
tests/integration/src/tests/end_to_end_workflows.rs (2)
crates/rock-node-core/src/block_writer.rs (2)
new(29-31)new(51-56)crates/rock-node-core/src/block_reader.rs (1)
new(22-24)
crates/rock-node-backfill-plugin/src/lib.rs (5)
crates/rock-node-backfill-plugin/examples/backfill_continious_mode.rs (1)
write_block_batch(61-67)crates/rock-node-backfill-plugin/src/worker.rs (1)
write_block_batch(749-756)crates/rock-node-core/src/block_writer.rs (2)
write_block_batch(19-19)write_block_batch(81-87)crates/rock-node-core/src/test_utils.rs (1)
write_block_batch(185-191)crates/rock-node-persistence-plugin/src/service.rs (1)
write_block_batch(200-242)
crates/rock-node-backfill-plugin/examples/backfill_continious_mode.rs (3)
crates/rock-node-backfill-plugin/src/lib.rs (2)
write_block(147-152)write_block_batch(154-161)crates/rock-node-core/src/block_writer.rs (4)
write_block(13-13)write_block(73-79)write_block_batch(19-19)write_block_batch(81-87)crates/rock-node-persistence-plugin/src/service.rs (2)
write_block(100-198)write_block_batch(200-242)
crates/rock-node-core/src/block_writer.rs (5)
crates/rock-node-backfill-plugin/examples/backfill_continious_mode.rs (2)
write_block(53-60)write_block_batch(61-67)crates/rock-node-backfill-plugin/src/lib.rs (3)
write_block(147-152)write_block_batch(154-161)new(27-32)crates/rock-node-backfill-plugin/src/worker.rs (7)
write_block(741-747)write_block_batch(749-756)new(190-226)new(619-621)new(670-676)new(720-725)new(843-853)crates/rock-node-core/src/test_utils.rs (5)
write_block(177-183)write_block_batch(185-191)new(105-109)new(157-161)get_written_blocks(163-167)crates/rock-node-persistence-plugin/src/service.rs (3)
write_block(100-198)write_block_batch(200-242)new(27-43)
crates/rock-node-persistence-plugin/src/service.rs (2)
crates/rock-node-core/src/block_writer.rs (4)
write_block(13-13)write_block(73-79)write_block_batch(19-19)write_block_batch(81-87)crates/rock-node-persistence-plugin/src/cold_storage/writer.rs (2)
get_block_number(113-122)make_block(131-147)
crates/rock-node-observability-plugin/src/lib.rs (3)
crates/rock-node-backfill-plugin/src/lib.rs (1)
create_test_context(168-226)crates/rock-node-state-management-plugin/src/plugin.rs (1)
create_test_context(231-251)tests/integration/src/fixtures/mock_plugins.rs (1)
create_test_context(103-120)
🪛 LanguageTool
charts/rock-node/README.md
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...age repository, tag, and any environment specific configuration as needed: ```ba...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Basic Quality Checks
- GitHub Check: Security Audit
🔇 Additional comments (51)
crates/rock-node-observability-plugin/src/lib.rs (2)
27-27: LGTM! Import needed for capability testing.The
Capabilityimport is correctly scoped to the test module and required for the new test that validates readiness behavior based on capability registration.
403-403: LGTM! Signature change improves testability.Changing the return type from
impl IntoResponseto the concrete tuple(StatusCode, &'static str)makes the function easier to test (as demonstrated in the new test) while maintaining compatibility with Axum's router, since the tuple implementsIntoResponse. This also improves consistency with thehealth_checkhandler (line 397).nginx.conf (2)
9-20: Verify the 24-hour timeouts and dual gRPC architecture.This stream block introduces raw TCP passthrough on port 6790 with 24-hour timeouts. Additionally, port 6790 has been moved from HTTP to stream context, which could break existing clients expecting HTTP/2 on this port.
Please verify:
- The 24-hour timeouts are appropriate for your use case and won't cause resource exhaustion under load
- Existing clients are updated to use port 6791 for HTTP/2 gRPC
- The dual architecture (raw TCP on 6790, HTTP/2 on 6791) is intentional and serves different client needs
review_comment_end -->
30-32: LGTM - Port change aligns with new stream configuration.The port change to 6791 and explicit HTTP/2 enablement are correct. This effectively separates raw TCP gRPC (port 6790) from HTTP/2 gRPC (port 6791), providing flexibility for different client implementations.
review_comment_end -->
crates/rock-node-state-management-plugin/src/plugin.rs (2)
27-27: LGTM: Enabled field additionThe
enabledfield is properly initialized tofalseand will be set during plugin initialization based on configuration. The field is private, which prevents external modification.Also applies to: 43-43
254-278: Excellent test coverage for disabled initializationThis test comprehensively verifies that when the plugin is disabled:
- The disabled state is recorded
- Internal components (
StateManager) are not created- Service providers (
StateReaderProvider) are not registered- The plugin does not start running
tests/integration/Cargo.toml (1)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No dependency changes; straightforward version update.
crates/rock-node-query-plugin/Cargo.toml (1)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No dependency changes; straightforward version update.
crates/rock-node-verifier-plugin/Cargo.toml (1)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No dependency changes; straightforward version update.
crates/rock-node-protobufs/Cargo.toml (2)
1-20: AI summary claims serde dependency removed, but it remains in the code.The AI-generated summary states "Removes the serde dependency from [dependencies]," but line 10 shows
serde = { version = "1", features = ["derive"] }is present. This is an inconsistency between the summary and the provided code snapshot. Please verify the actual state of the serde dependency in this PR.
3-3: Version bump aligns with workspace-wide 0.2.1 release.Aside from the serde dependency inconsistency flagged above, the version bump is straightforward and coordinated with the workspace.
crates/rock-node-publish-plugin/Cargo.toml (1)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No dependency changes; straightforward version update.
crates/rock-node-block-access-plugin/Cargo.toml (1)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No dependency changes; straightforward version update.
app/rock-node/Cargo.toml (1)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No dependency changes; straightforward version update.
tests/e2e/Cargo.toml (2)
3-3: Version bump aligns with workspace-wide 0.2.1 release.No issues with the crate version update.
13-13: No breaking changes detected—upgrade is safe.The testcontainers v0.25 breaking changes (bollard update, removed features like hickory dns, weak-deps) do not affect the test code. The e2e tests use only stable public APIs (GenericImage, ImageExt, AsyncRunner, Mount, WaitFor, etc.) and contain no direct bollard dependencies. No code updates are required.
crates/rock-node-server-status-plugin/Cargo.toml (1)
3-3: LGTM! Version bump aligns with workspace.The version bump to 0.2.1 is consistent with the workspace-wide version update.
Cargo.toml (1)
21-21: LGTM! Workspace version bump to 0.2.1.The workspace version is updated consistently across all member crates.
crates/rock-node-observability-plugin/Cargo.toml (1)
3-3: LGTM! Version bump aligns with workspace.The version bump to 0.2.1 is consistent with the workspace-wide version update.
crates/rock-node-subscriber-plugin/Cargo.toml (1)
3-3: LGTM! Version bump aligns with workspace.The version bump to 0.2.1 is consistent with the workspace-wide version update.
crates/rock-node-backfill-plugin/Cargo.toml (2)
3-3: LGTM! Version bump aligns with workspace.The version bump to 0.2.1 is consistent with the workspace-wide version update.
32-32: Verify RocksDB column family API compatibility with 0.24 release before merging.The breaking change between 0.22 and 0.24 regarding column family options does impact this codebase. The plugin uses column family operations at lines 544-545 and 1004-1006 (cf_handle and iterator_cf with CF_GAPS), which are directly affected by the breaking change. Test the code with RocksDB 0.24 to confirm the column family API calls remain compatible and that existing databases can be opened without migration.
crates/rock-node-state-management-plugin/Cargo.toml (1)
3-3: LGTM! Version bump aligns with workspace.The version bump to 0.2.1 is consistent with the workspace-wide version update.
crates/rock-node-persistence-plugin/Cargo.toml (1)
3-3: LGTM! Version and dependency updates are consistent.The version bump to 0.2.1 and rocksdb upgrade to 0.24 align with the workspace-wide updates described in the PR summary.
Also applies to: 23-23
charts/rock-node/templates/configmap.yaml.tpl (1)
1-15: LGTM! Well-structured ConfigMap template.The template correctly implements conditional rendering based on
existingConfigMapand uses proper Helm template functions for labels and content indentation.charts/rock-node/templates/pvc.yaml.tpl (1)
1-29: LGTM! Properly structured PVC template.The template correctly implements conditional persistence with support for existing claims and includes all standard PVC configuration options.
crates/rock-node-core/Cargo.toml (1)
3-3: LGTM! Version and dependency updates are consistent.The version bump to 0.2.1 and rocksdb upgrade to 0.24 align with the workspace-wide changes.
Also applies to: 13-13
charts/rock-node/templates/service.yaml.tpl (1)
1-36: LGTM! Well-designed Service template.The template correctly handles different service types and conditionally includes nodePort only when appropriate (NodePort or LoadBalancer types). The metrics port is properly gated by the enabled flag.
crates/rock-node-persistence-plugin/src/lib.rs (2)
302-302: LGTM! Correct usage of Arc for the updated API.The change to
Arc::new(block_proto)correctly aligns with the updatedBlockWriter::write_blocksignature that now acceptsArc<Block>instead of&Block.
210-218: Review comment is incorrect and should be ignored.The review comment fails to account for critical constraints:
The
initializemethod is synchronous by trait design. The Plugin trait requiresfn initialize(&mut self, context: AppContext) -> Result<()>, notasync fn. Making it async would be a breaking API change affecting all plugins.Cannot use
.awaitin a synchronous function. The first suggested fix is impossible without violating the trait contract.The
registermethod cannot fail. Its signature ispub async fn register(&self, capability: Capability)returning unit, notResult. There is no error case to handle, making the second suggestion moot.Fire-and-forget spawn is the correct pattern. Given that:
initializemust be synchronous (trait requirement)- The operation is instantaneous (lock acquisition + HashSet insert)
- No failures are possible
- The same pattern is used consistently in other plugins (verifier-plugin, persistence-plugin)
The current implementation appropriately delegates the async work to a background task, which is the standard pattern when an async operation must be initiated from a synchronous context.
Likely an incorrect or invalid review comment.
crates/rock-node-backfill-plugin/src/worker.rs (3)
533-533: LGTM! Correct Arc usage for the updated BlockWriter API.The change properly wraps the block in
Arc::new()to match the updatedwrite_blocksignature.
594-594: LGTM! Consistent Arc usage in test helper.The test helper correctly wraps blocks in
Arc::new()to match the updated API.
741-741: LGTM! Mock implementations updated correctly.The mock
BlockWriterimplementation has been properly updated to acceptArc<Block>andArc<Vec<Block>>parameters, aligning with the trait signature changes.Also applies to: 749-749
crates/rock-node-backfill-plugin/src/lib.rs (1)
147-162: LGTM! Arc-based signatures align with the trait refactor.The mock BlockWriter correctly adopts
Arc<Block>andArc<Vec<Block>>signatures, consistent with the broader migration to Arc-based ownership semantics across the codebase.charts/rock-node/values.yaml (2)
152-161: LGTM! Persistence configuration is sensible.The default 100Gi size with ReadWriteOnce access mode is appropriate for single-pod block storage deployments. Empty
storageClassNamecorrectly delegates to the cluster default.
138-151: Probe endpoints are properly implemented and configured.Both
/readyzand/livezendpoints are correctly implemented incrates/rock-node-observability-plugin/src/lib.rswith handlers that return appropriate HTTP status codes (200 OK when healthy, 503 SERVICE_UNAVAILABLE when not ready). The port 9600 is the correct default observability server port configured across the codebase. E2E tests confirm the endpoints are functional.tests/integration/src/tests/end_to_end_workflows.rs (1)
8-502: LGTM! Test updates correctly adopt Arc-based BlockWriter API.All test calls to
write_blockandwrite_block_batchhave been systematically updated to wrap blocks withArc::new(), consistent with the refactored BlockWriter trait signatures. The changes maintain test correctness while enabling shared ownership semantics.charts/rock-node/templates/ingress.yaml.tpl (1)
40-43: Port fallback for custom backends is technically valid but appears to be an unintended code path.The review comment's concern is accurate: lines 40-43 use a custom backend name but default to
$.Values.service.grpc.port. However, this appears to be a fallback for an unsupported scenario rather than a documented feature—no ingress examples, documentation, or default configurations show custom backends being used. The ingress is disabled by default with an empty hosts list.The concern remains valid for any user who attempts to configure custom backends, as they would need to match the gRPC port or face routing failures. The template should either:
- Document that only "grpc" and "metrics" are supported
- Add a mechanism to specify ports for custom backends
- Remove the else clause if custom backends are not intended
charts/rock-node/templates/_helpers.tpl (1)
1-32: LGTM! Standard Helm helper templates following best practices.The helper templates follow Helm conventions:
- Proper truncation to 63 characters for Kubernetes naming constraints
- Standard fallback patterns for name overrides
- Correct service account name resolution logic
- Namespace override support
crates/rock-node-core/src/test_utils.rs (2)
185-191: LGTM! Batch write implementation correctly uses Arc.The
extend_from_slicewith&blockscorrectly leverages the Arc-wrapped Vec without unnecessary cloning.
253-263: LGTM! Test correctly uses Arc-wrapped blocks.The test properly wraps the block in Arc before passing to write_block.
crates/rock-node-backfill-plugin/examples/backfill_continious_mode.rs (1)
52-68: LGTM! Example correctly implements Arc-based BlockWriter signatures.The example implementations properly accept Arc-wrapped blocks, aligning with the updated BlockWriter trait. The logging demonstrates the usage pattern without actual persistence logic, which is appropriate for an example.
charts/rock-node/templates/deployment.yaml.tpl (1)
1-128: LGTM! Comprehensive deployment template with proper conditional rendering.The deployment template properly:
- Uses helper functions for naming and labels
- Conditionally renders security contexts, probes, and volumes
- Supports persistence with PVC
- Includes proper resource limits and node selectors
crates/rock-node-persistence-plugin/src/service.rs (3)
99-197: LGTM! Efficient Arc-based write_block implementation.The implementation correctly:
- Accepts Arc to avoid cloning large protobuf messages
- Captures Arc-cloned values in spawn_blocking (line 112 comment is accurate)
- Dereferences Arc only when needed (e.g.,
get_block_number(&block))- Maintains all gap-filling and state management logic
The Arc migration is clean and preserves the original behavior while improving efficiency.
199-242: LGTM! Efficient Arc-based write_block_batch implementation.The batch write correctly:
- Accepts Arc<Vec> to avoid cloning the entire batch
- Passes the Arc-wrapped batch to the archiver (line 217 comment is accurate)
- Updates cold reader index and state appropriately
The Arc migration maintains the same behavior with better memory efficiency.
313-346: LGTM! Tests updated to use Arc-wrapped blocks.All test cases properly wrap blocks in Arc::new() before passing to write_block and write_block_batch, demonstrating correct usage of the updated API.
crates/rock-node-core/src/block_writer.rs (3)
10-20: LGTM! Clear trait definition with Arc-based ownership.The BlockWriter trait correctly uses Arc for both single blocks and batches to avoid expensive cloning of large protobuf messages. The comments clearly document this design decision.
80-88: LGTM! Batch write correctly uses Arc without unnecessary cloning.The
extend_from_slice(&blocks)leverages the Arc-wrapped Vec efficiently.
90-313: LGTM! Comprehensive test coverage for Arc-based BlockWriter.All tests properly:
- Wrap blocks in Arc::new() before passing to write methods
- Test single writes, batches, concurrent access, and failure scenarios
- Verify provider behavior and thread safety
The test suite thoroughly validates the Arc-based API changes.
crates/rock-node-core/src/cache.rs (2)
105-111: Keep deferred deletion to avoid reintroducing the race.The struct’s own documentation still calls out the deferred deletion mechanism as the way we avoid races in the pipeline. By removing the entry immediately, any consumer that receives just the UUID (before it has had a chance to clone the data) will now observe
None, which is the race the queue-based cleanup was designed to prevent. This can drop in-flight blocks under load.Please revert to enqueuing the key for cleanup (or otherwise ensure the key stays visible until the background task processes it) so we keep the original safety property.
186-191: Test now codifies the regression.Because
mark_for_removalshould remain deferred, this test shouldn’t expect the entry to disappear immediately. The new assertion makes the race mandatory and will hide the bug above. Please restore the previous expectation (entry still reachable until cleanup).
georgi-l95
force-pushed
the
overall-improvements
branch
from
98c1f2d to
d624e4d
Compare
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
georgi-l95
force-pushed
the
overall-improvements
branch
from
d624e4d to
52321f8
Compare
Signed-off-by: georgi-l95 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
+ Coverage 80.19% 80.27% +0.07%
==========================================
Files 56 56
Lines 10584 10621 +37
==========================================
+ Hits 8488 8526 +38
+ Misses 2096 2095 -1
🚀 New features to boost your workflow:
|
Description
This PR improves persistence memory usage and bumps dependencies
Reviewer Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores