Replace NodeID field access with ID() #4784
Conversation
WalkthroughThis pull request introduces a systematic refactoring across multiple files to replace direct access to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/nats/proxy/management_proxy.go (1)
Line range hint
71-71: Consider updating remaining NodeID usages for consistency.The
UpdateInfoandUpdateResourcesmethods still use directNodeIDaccess whileRegisterusesID(). Consider updating these for consistency.- asyncRes, err = send[legacy.UpdateInfoRequest, legacy.UpdateInfoResponse]( - ctx, p.conn, request.Info.NodeID, request, UpdateNodeInfo) + asyncRes, err = send[legacy.UpdateInfoRequest, legacy.UpdateInfoResponse]( + ctx, p.conn, request.Info.ID(), request, UpdateNodeInfo) - asyncRes, err = send[legacy.UpdateResourcesRequest, legacy.UpdateResourcesResponse]( - ctx, p.conn, request.NodeID, request, UpdateResources) + asyncRes, err = send[legacy.UpdateResourcesRequest, legacy.UpdateResourcesResponse]( + ctx, p.conn, request.Info.ID(), request, UpdateResources)Also applies to: 89-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/models/node_info.go(0 hunks)pkg/nats/proxy/management_proxy.go(1 hunks)pkg/orchestrator/selection/ranking/available_capacity.go(2 hunks)pkg/transport/bprotocol/orchestrator/heartbeat_test.go(1 hunks)pkg/transport/nclprotocol/compute/controlplane.go(1 hunks)pkg/transport/nclprotocol/compute/controlplane_test.go(1 hunks)pkg/transport/nclprotocol/compute/manager_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/models/node_info.go
🔇 Additional comments (6)
pkg/nats/proxy/management_proxy.go (1)
55-55: LGTM! Consistent with PR objective.
The change from request.Info.NodeID to request.Info.ID() aligns with the encapsulation goal.
pkg/orchestrator/selection/ranking/available_capacity.go (1)
106-107: LGTM! Consistent NodeID access pattern.
The changes correctly replace direct NodeID access with ID() method calls in both the capacity calculation and ranking logic, maintaining consistency with the encapsulation goal.
Also applies to: 127-128
pkg/transport/bprotocol/orchestrator/heartbeat_test.go (1)
227-227: LGTM! Consistent with production code changes.
The test correctly uses the ID() method to retrieve node state, maintaining consistency with the production code changes.
pkg/transport/nclprotocol/compute/manager_test.go (2)
162-162: LGTM: Test assertion updated to use ID() method
The test correctly verifies heartbeat message construction using the ID() method.
181-181: LGTM: Consistent usage of ID() method in subsequent test verification
The change maintains consistency in how node identifiers are accessed throughout the test suite.
pkg/transport/nclprotocol/compute/controlplane_test.go (1)
161-161: LGTM: Test updated to use ID() method in heartbeat message construction
The test correctly reflects the implementation change by using the ID() method to construct the test message.
| @@ -139,7 +139,7 @@ func (cp *ControlPlane) heartbeat(ctx context.Context) error { | |||
| cp.latestNodeInfo = nodeInfo | |||
|
|
|||
| msg := envelope.NewMessage(messages.HeartbeatRequest{ | |||
| NodeID: cp.latestNodeInfo.NodeID, | |||
| NodeID: cp.latestNodeInfo.ID(), | |||
There was a problem hiding this comment.
💡 Codebase verification
Found another NodeID field access that needs to be updated
There is another direct NodeID field access in the ShutdownNoticeRequest message construction that should be updated to use the ID() method for consistency:
- Line containing
NodeID: cp.cfg.NodeIDin ShutdownNoticeRequest message construction
🔗 Analysis chain
LGTM: NodeID field access correctly replaced with ID() method
The change aligns with the PR objective of using the ID() method instead of direct field access.
Let's verify there are no remaining direct NodeID field accesses in this file:
Let's gather more specific information about the remaining NodeID usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct NodeID field accesses
rg -l "\.NodeID\b" pkg/transport/nclprotocol/compute/controlplane.go
Length of output: 120
Script:
#!/bin/bash
# Search for all occurrences of NodeID with surrounding context
rg -A 2 -B 2 "\.NodeID\b" pkg/transport/nclprotocol/compute/controlplane.go
Length of output: 334
…method-across-the
wdbaruni
deleted the
eng-307-replace-nodeid-field-access-with-id-method-across-the
branch
Summary by CodeRabbit
New Features
Bug Fixes
Tests