-
Notifications
You must be signed in to change notification settings - Fork 4
Feature/service discovery #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a service discovery system that enables nodes to advertise and discover services from other nodes in the network. The implementation includes a database-backed service registry, a pub/sub mechanism for broadcasting service changes, and shell operations for querying and synchronizing service information.
- Adds a new
servicesmodule with discovery, persistence, and synchronization capabilities - Implements a
ServiceChangeFeedpub/sub mechanism for broadcasting service state changes - Integrates the NAT module as an example service discoverer
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mod/services/module.go | Core interface defining the services module API and discovery options |
| mod/services/service.go | Service data structure representing a discoverable service |
| mod/services/service_change.go | Event type for service state changes (enabled/disabled) |
| mod/services/service_change_feed.go | Pub/sub broadcast primitive for service change events |
| mod/services/src/module.go | Main module implementation with remote service discovery logic |
| mod/services/src/op_discovery.go | Shell operation handler for discovering local services with snapshot and follow modes |
| mod/services/src/op_sync.go | Shell operation handler for synchronizing services from a remote identity |
| mod/services/src/loader.go | Module loader with database migration and dependency injection |
| mod/services/src/deps.go | Dependency injection setup that discovers ServiceDiscoverer implementations |
| mod/services/src/db.go | Database operations layer with transaction support |
| mod/services/src/db_service.go | Database model for cached service records |
| mod/nat/src/service_discoverer.go | NAT module's implementation of ServiceDiscoverer interface |
| mod/nat/src/module.go | Added service discovery fields to NAT module |
| mod/nat/src/loader.go | Initialize ServiceChangeFeed in NAT module loader |
| mod/all/mods.go | Register the new services module |
Comments suppressed due to low confidence (1)
mod/nat/src/module.go:45
- The serviceChangeFeed is created in the loader but is never closed when the module shuts down. The Run method (line 41-45) doesn't call Close on the feed before returning. This could lead to resource leaks, particularly goroutines from Subscribe calls that may not properly clean up. Consider adding a defer statement to close the feed when the context is done.
func (mod *Module) Run(ctx *astral.Context) error {
mod.ctx = ctx.IncludeZone(astral.ZoneNetwork)
<-ctx.Done()
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
81486d5 to
9d7fb5c
Compare
|
@cryptoarashi rebased on master to use new channel implementation |
cryptoarashi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Ops should not contain heavy logic.
Move heavy logic to a module function DiscoverServices(...) (<-chan, error) and
use it in an op. See mod/objects the scope of OpSearch() vs Search(),
OpDescribe() vs Describe().
-
A single module should be able to expose more than one service. This implementation assumes all services will only post a single service in its snapshot collecting phase.
-
Object files are missing the init function adding them to the default blueprints.
Done @cryptoarashi |
No description provided.