Validate dependsOn Field #477
Conversation
WalkthroughThe pull request refactors cursor management across multiple components by introducing a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
mkysel
force-pushed
the
mkysel/dependsonvalidation
branch
from
957ddcb to
e2c21cf
Compare
![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 (6)
pkg/api/metadata/cursorUpdater.go (6)
14-19: Define interface contracts in comments for clarity.
It can be helpful to add a short docstring on each interface method explaining the high-level purpose—especially forStop(), which is not immediately apparent from the name, to guide future contributors.
21-26: Ensure robust error handling and clarify concurrency model in constructor.
- The new fields
cancelandwgfacilitate graceful shutdown, but consider clarifying their usage in the struct-level comments.- Inside
NewCursorUpdater, any failure inside the goroutine started bytracing.GoPanicWrapwill exit quietly if not properly handled. A structured error-handling approach or retry logic might be valuable.Also applies to: 33-41, 45-51
61-79: Consider reacting to DB changes on-demand rather than a fixed ticker.
Although the 100 ms ticker works, it may incur unnecessary DB reads. If feasible, a DB trigger or a more event-driven approach could reduce overhead, especially under production load.
93-113: Add structured error handling and logging.
TheTODO proper error handlingcomment indicates the need for a safer fallback or a backoff strategy. Logging the error and continuing vs. completely exiting might be preferable depending on business requirements.
115-129: Watch out for channel blocking innotifySubscribers().
If many subscribers exist and some are slow to receive, the loop might hold the lock for longer than desired. While the current implementation is acceptable, consider a more concurrent approach if usage grows.
137-141: Ensure explicit removal scenarios.
Clients that do not remove themselves (lack ofRemoveSubscribercalls) can accumulate. If that’s acceptable, clarify usage in documentation. Otherwise, consider a time-based cleanup or heartbeat mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/api/message/service.go(7 hunks)pkg/api/metadata/cursorUpdater.go(4 hunks)pkg/api/metadata/service.go(1 hunks)pkg/server/server.go(4 hunks)pkg/testutils/api/api.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (9)
pkg/api/metadata/cursorUpdater.go (3)
55-59: Avoid returning a pointer to internal state if mutation is a concern.
Returning*envelopes.Cursoris fine, but keep in mind that future code might inadvertently mutate the returned struct. If that’s a concern, you could either clone the map or use a copy constructor.
131-135: Confirm safe handling for concurrent additions.
Locking is used here, which is good. However, if subscribers are frequently added or removed, consider verifying performance impact under high concurrency.
142-145: Graceful shutdown is well-implemented.
Callingcancel()and thenwg.Wait()ensures no new operations occur while waiting for the existing goroutine to finish, preventing resource leaks.pkg/api/metadata/service.go (1)
18-19: Confirm that storing the interface by value is intentional.
Usingcu CursorUpdateravoids an extra level of indirection, which is usually fine. However, confirm that centralized updates to the cursor are still reflected across consumers.Also applies to: 24-25, 29-29
pkg/testutils/api/api.go (1)
127-127: Reusingmetadata.NewCursorUpdateris consistent.
Passing the same constructor into both replication and metadata services ensures a single source for cursor updates, aligning with the new architecture. Just confirm no separate, conflicting updaters are inadvertently created elsewhere.Also applies to: 143-147
pkg/server/server.go (3)
50-50: LGTM! Field addition is well-placed.The
cursorUpdaterfield is appropriately added to theReplicationServerstruct, enabling cursor management functionality.
187-187: LGTM! Proper initialization and dependency injection.The cursor updater is correctly initialized and injected into both the replication and metadata services, ensuring consistent cursor management across services.
Also applies to: 189-196, 204-208
286-288: LGTM! Clean resource management.The
Shutdownmethod properly cleans up the cursor updater resources, preventing potential resource leaks.pkg/api/message/service.go (1)
44-44: LGTM! Clean integration of cursor updater.The
cufield is appropriately added to theServicestruct, enabling cursor validation functionality.
| @@ -449,7 +453,25 @@ func (s *Service) validateClientInfo(clientEnv *envelopes.ClientEnvelope) error | |||
| return status.Errorf(codes.InvalidArgument, "topic does not match payload") | |||
| } | |||
|
|
|||
| // TODO(rich): Verify all originators have synced past `last_seen` | |||
| if aad.GetDependsOn() != nil { | |||
There was a problem hiding this comment.
LGTM.
Should we remove the aad.GetTargetOriginator() != s.registrant.NodeID() check on this PR?
There was a problem hiding this comment.
Yeah. That check will have to go. I need to introduce the new field set by the payer. I suspect we can still validate the originator if it exists for backwards compatibility?
There was a problem hiding this comment.
Burn the boats. We're too early to worry about backward compatibility for things like validations that don't require coordination between nodes..
There was a problem hiding this comment.
It's tracked here #463
Summary by CodeRabbit
New Features
Refactor
Tests