Repository navigation
refactor #326: tighten public API surface across d-engine crates - #363
Merged
Merged
Conversation
- d-engine-core: replace wildcard re-exports with explicit user-facing list; add #[doc(hidden)] to 11 internal error types; delete dead QuorumStatus enum - d-engine-server: restrict node_config, set_rpc_ready, is_rpc_ready to pub(crate); delete unused ready_notifier() and from_raft(); fix EmbeddedEngine::node_id() to use stored field instead of client_id; add #[doc(hidden)] to HardState, ProstError, SnapshotError re-exports - d-engine-client: restrict ClientInner and pool re-export to pub(crate); remove utils wildcard re-export; delete three inherent get_linearizable/get_lease/get_eventual methods that shadowed ClientApi trait and returned Option<ClientResult> instead of Option<Bytes> - NodeBuilder: fix node_config() setter bug (missing node_id sync); rename init() to pub(crate); integration tests updated to use NodeBuilder::new().node_config() - examples/docs: update service-discovery watcher and dengine_ctl to new API; add MIGRATION_GUIDE.md for v0.2.3->v0.2.4 breaking changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR tightens the public API for v0.2.3 → v0.2.4: it hides internal types and modules, removes several public Node/Client convenience methods and fields, moves node-id ownership to EmbeddedEngine, updates NodeBuilder construction, and adjusts tests/examples to the new public surface. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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
🧹 Nitpick comments (3)
d-engine-server/src/lib.rs (1)
129-135: Keep storage support types discoverable.
HardStateis still part of the public custom-storage flow viaMetaStore, so hiding the crate-root export makes that path harder to follow from docs.rs. If you want it out of the main API section, a documentedstorage_typesmodule would be easier to discover than a#[doc(hidden)]re-export.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-server/src/lib.rs` around lines 129 - 135, Remove the #[doc(hidden)] on the HardState re-export so it remains discoverable for the public custom-storage flow (MetaStore); either keep HardState public at crate root or move the re-exports into a documented module (e.g., pub mod storage_types) and re-export HardState (and related types like ProstError and SnapshotError) there with a module-level doc comment so users can find them on docs.rs.d-engine-core/src/lib.rs (1)
97-125: Move hidden internals behind a dedicated namespace.
#[doc(hidden)] pub use ...::*still leaves these symbols reachable asd_engine_core::..., so external code can keep binding to the old root paths. If the goal is to make the smaller surface real, consider re-exporting them from aninternalmodule and pointingd-engine-serverat that instead.♻️ Possible shape
-#[doc(hidden)] -pub use commit_handler::*; -#[doc(hidden)] -pub use election::*; +#[doc(hidden)] +pub mod internal { + pub use super::commit_handler::*; + pub use super::election::*; + // ...other internal-only re-exports... +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/lib.rs` around lines 97 - 125, The current #[doc(hidden)] pub use ...::* lines leave internals accessible at the crate root (e.g., commit_handler, election, event, maybe_clone_oneshot, membership, network, purge, raft_context, raft_role, replication, state_machine_handler, type_config, utils); move them behind a dedicated hidden namespace by creating a module (e.g., #[doc(hidden)] pub mod internal) and re-exporting those modules inside it (replace root-level pub use with pub use inside internal), then update d-engine-server to import from d_engine_core::internal::... so external crates cannot continue to bind to the old root paths.MIGRATION_GUIDE.md (1)
369-370: Optional docs polish: clarify crate path forClientApiimport.Line 369 currently shows
d_engine_client::ClientApi; consider noting the equivalent facade import (d_engine::client::ClientApi) if both are supported, to reduce migration ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MIGRATION_GUIDE.md` around lines 369 - 370, Update the docs to clarify the crate path for ClientApi by noting both import options: the direct crate import (d_engine_client::ClientApi) and the facade path (d_engine::client::ClientApi), and show the example line using either form so readers know they can call client.get_linearizable("key").await? with the ClientApi trait brought into scope via either import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d-engine-server/src/api/embedded_client.rs`:
- Around line 154-158: The # Errors docblocks for the put and delete methods are
too narrow; update the documentation for both methods (the put and delete
functions in embedded_client.rs) to state that errors may include not only
leader/not-leader, closed channel, or timeout, but also RPC-status failures and
other server-side errors (including non-NotLeader errors); apply the same
broadened wording to both methods' # Errors sections so they accurately reflect
RPC and server error surface.
In `@d-engine-server/src/node/builder.rs`:
- Around line 173-176: The constructor that accepts a ready-made RaftNodeConfig
was made crate-private as init, which forces callers to go through
NodeBuilder::new(...).node_config(...) and triggers new()'s expect path; add a
public constructor pub fn from_node_config(node_config: RaftNodeConfig,
shutdown_signal: watch::Receiver<()>) -> Self that performs the same
initialization as init (or factor the shared logic into a private helper used by
both init and the new public from_node_config) so callers can supply a pre-built
RaftNodeConfig without hitting the panic path; update any docs/examples that
referenced init to use RaftNodeBuilder::from_node_config (or the correct public
function name) and ensure symbol names match RaftNodeConfig, NodeBuilder::new,
node_config, and init.
---
Nitpick comments:
In `@d-engine-core/src/lib.rs`:
- Around line 97-125: The current #[doc(hidden)] pub use ...::* lines leave
internals accessible at the crate root (e.g., commit_handler, election, event,
maybe_clone_oneshot, membership, network, purge, raft_context, raft_role,
replication, state_machine_handler, type_config, utils); move them behind a
dedicated hidden namespace by creating a module (e.g., #[doc(hidden)] pub mod
internal) and re-exporting those modules inside it (replace root-level pub use
with pub use inside internal), then update d-engine-server to import from
d_engine_core::internal::... so external crates cannot continue to bind to the
old root paths.
In `@d-engine-server/src/lib.rs`:
- Around line 129-135: Remove the #[doc(hidden)] on the HardState re-export so
it remains discoverable for the public custom-storage flow (MetaStore); either
keep HardState public at crate root or move the re-exports into a documented
module (e.g., pub mod storage_types) and re-export HardState (and related types
like ProstError and SnapshotError) there with a module-level doc comment so
users can find them on docs.rs.
In `@MIGRATION_GUIDE.md`:
- Around line 369-370: Update the docs to clarify the crate path for ClientApi
by noting both import options: the direct crate import
(d_engine_client::ClientApi) and the facade path (d_engine::client::ClientApi),
and show the example line using either form so readers know they can call
client.get_linearizable("key").await? with the ClientApi trait brought into
scope via either import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c24c746c-8073-43c8-bc1d-0568bd5b74fd
📒 Files selected for processing (19)
MIGRATION_GUIDE.mdd-engine-client/src/grpc_client.rsd-engine-client/src/grpc_client_test.rsd-engine-client/src/lib.rsd-engine-client/src/utils_test.rsd-engine-core/src/errors.rsd-engine-core/src/lib.rsd-engine-server/src/api/embedded.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/lib.rsd-engine-server/src/node/builder.rsd-engine-server/src/node/mod.rsd-engine-server/src/node/node_test.rsd-engine-server/tests/cas_operations/distributed_lock_standalone.rsd-engine-server/tests/cas_operations/snapshot_recovery_standalone.rsd-engine-server/tests/common/mod.rsd-engine-server/tests/failover_and_recovery/leader_failover_standalone.rsexamples/service-discovery-standalone/watcher.rsexamples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/src/main.rs
💤 Files with no reviewable changes (1)
- d-engine-client/src/grpc_client.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…r and election config - Add NodeBuilder::from_node_config() as a panic-safe public constructor accepting a pre-built RaftNodeConfig directly, avoiding the implicit RaftNodeConfig::new() detour in new() that can panic in environments without a default config file - Add two unit tests (TDD): test_from_node_config_preserves_caller_config verifies caller's config is used intact; test_node_config_setter_syncs_node_id covers the node_id sync bug fixed in #326 - Update module-level and start() doc examples to reference from_node_config instead of the now-crate-private init() - Broaden # Errors in EmbeddedClient::put() and delete() to include state machine server errors - Add [raft.election] and [retry.election] to create_node_config() and create_node_config_with_role() TOML output so embedded integration tests get CI-stable election settings (election_timeout_max 3000ms, retry.election.timeout_ms 2000ms) instead of narrow defaults that cause flaky leader-failover tests on loaded CI machines Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
d-engine-server/tests/common/mod.rs (1)
132-140: Centralize duplicated election/retry test tuning values.These values are now duplicated in generated TOML and in
node_config(...); extracting shared constants/helper config will reduce drift risk across tests.Also applies to: 184-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-server/tests/common/mod.rs` around lines 132 - 140, Extract the duplicated election and retry tuning values into shared test constants or a helper struct (e.g., ELECTION_TIMEOUT_MIN, ELECTION_TIMEOUT_MAX, RETRY_MAX_RETRIES, RETRY_TIMEOUT_MS, RETRY_BASE_DELAY_MS, RETRY_MAX_DELAY_MS) and update both the generated TOML snippets and the node_config(...) call to reference those constants/helper instead of hard-coding the numbers; ensure the helper is defined in the tests/common/mod.rs module and used in both places so future changes stay centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d-engine-server/src/node/builder_test.rs`:
- Around line 285-299: The test test_node_config_setter_syncs_node_id currently
only asserts builder.node_config.cluster.node_id but the comment claims the
internal node_id is synced too; update the test in
NodeBuilder::<MockStorageEngine, MockStateMachine>::new(...) usage to also
assert the builder's internal node_id field (e.g., builder.node_id) equals 7 so
the setter behavior is actually validated; locate the test function
test_node_config_setter_syncs_node_id and add a second assertion that compares
builder.node_id to 7.
---
Nitpick comments:
In `@d-engine-server/tests/common/mod.rs`:
- Around line 132-140: Extract the duplicated election and retry tuning values
into shared test constants or a helper struct (e.g., ELECTION_TIMEOUT_MIN,
ELECTION_TIMEOUT_MAX, RETRY_MAX_RETRIES, RETRY_TIMEOUT_MS, RETRY_BASE_DELAY_MS,
RETRY_MAX_DELAY_MS) and update both the generated TOML snippets and the
node_config(...) call to reference those constants/helper instead of hard-coding
the numbers; ensure the helper is defined in the tests/common/mod.rs module and
used in both places so future changes stay centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ba33432-1c72-40c8-b1ed-c8c0bf68a737
📒 Files selected for processing (4)
d-engine-server/src/api/embedded_client.rsd-engine-server/src/node/builder.rsd-engine-server/src/node/builder_test.rsd-engine-server/tests/common/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- d-engine-server/src/api/embedded_client.rs
- d-engine-server/src/node/builder.rs
Comment on lines
+285
to
+299
| #[test] | ||
| fn test_node_config_setter_syncs_node_id() { | ||
| let (_, shutdown_rx) = watch::channel(()); | ||
| let mut config = RaftNodeConfig::new().unwrap().validate().unwrap(); | ||
| config.cluster.node_id = 7; | ||
|
|
||
| let builder = NodeBuilder::<MockStorageEngine, MockStateMachine>::new(None, shutdown_rx) | ||
| .node_config(config); | ||
|
|
||
| // node_config on the builder must reflect the new config after setter | ||
| assert_eq!( | ||
| builder.node_config.cluster.node_id, 7, | ||
| "node_config() setter must sync node_id into both node_config and internal node_id field" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test does not assert the internal node_id it claims to validate.
The test message says both node_config and internal node_id are synced, but it only checks builder.node_config.cluster.node_id. Add an assertion for builder.node_id (or an equivalent observable) to actually lock this behavior.
Proposed test fix
#[test]
fn test_node_config_setter_syncs_node_id() {
let (_, shutdown_rx) = watch::channel(());
let mut config = RaftNodeConfig::new().unwrap().validate().unwrap();
config.cluster.node_id = 7;
let builder = NodeBuilder::<MockStorageEngine, MockStateMachine>::new(None, shutdown_rx)
.node_config(config);
// node_config on the builder must reflect the new config after setter
assert_eq!(
builder.node_config.cluster.node_id, 7,
"node_config() setter must sync node_id into both node_config and internal node_id field"
);
+ assert_eq!(
+ builder.node_id, 7,
+ "node_config() setter must sync internal node_id field"
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@d-engine-server/src/node/builder_test.rs` around lines 285 - 299, The test
test_node_config_setter_syncs_node_id currently only asserts
builder.node_config.cluster.node_id but the comment claims the internal node_id
is synced too; update the test in NodeBuilder::<MockStorageEngine,
MockStateMachine>::new(...) usage to also assert the builder's internal node_id
field (e.g., builder.node_id) equals 7 so the setter behavior is actually
validated; locate the test function test_node_config_setter_syncs_node_id and
add a second assertion that compares builder.node_id to 7.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This PR Do?
Tightens the public API surface across all d-engine crates by hiding
internal types, restricting visibility, and removing methods that leaked
implementation details or created semantic traps for downstream users.
Type:
Why Is This Needed?
For bugs: See #326 for full issue list. Key problems fixed:
GrpcClienthad three inherent methods (get_linearizable,get_lease,get_eventual) returningOption<ClientResult>that silently shadowed theClientApitrait methods returningOption<Bytes>— callers got the wrongtype depending on whether
ClientApiwas in scopeEmbeddedEngine::node_id()returnedclient_idinstead of the actual nodeID (wrong field)
NodeBuilder::node_config()setter did not syncself.node_id, causingnodes to boot with wrong IDs (reproduced as connection timeout in
test_distributed_lock_standalone)d-engine-coreand internal types visible incargo docmade the public API surface appear ~3x larger than intendedChecklist
Required:
make testpassesIf changing APIs:
Testing
How tested:
test_ready_notifier_independentrewritten as
test_rpc_ready_and_leader_election_are_independentafterready_notifier()was deleteddistributed_lock_standalone,snapshot_recovery_standalone,leader_failover_standalone— all updated and passingcargo doc --no-depsreviewed to confirm internal typesno longer appear in generated docs
For bug fixes:
NodeBuildernode_id sync bug is covered by existing distributed lockintegration test (was failing before fix, passes after)
Does This Follow d-engine's Principles?
Reviewer Notes
Breaking changes — see
MIGRATION_GUIDE.mdfor v0.2.3 → v0.2.4:GrpcClient::get_linearizable/get_lease/get_eventualremoved from inherentimpl; bring
ClientApiinto scope to use trait methods (return type is nowOption<Bytes>notOption<ClientResult>)NodeBuilder::init()→pub(crate); useNodeBuilder::new().node_config()RaftNode::node_configfield →pub(crate); usenode.node_id()accessorRaftNode::ready_notifier(),from_raft()deletedRaftNode::set_rpc_ready(),is_rpc_ready()→pub(crate)d-engine-corewildcard re-exports replaced with explicit list; importpaths unchanged but
#[doc(hidden)]on internal typesd-engine-serverre-exportsHardState,ProstError,SnapshotErrornow
#[doc(hidden)]Estimated review complexity:
Summary by CodeRabbit
Breaking Changes
Documentation
Tests