Adds support for tree configurations#328
Conversation
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go | May 12, 2026 9:35p.m. | Review ↗ | |
| Shell | May 12, 2026 9:35p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
There was a problem hiding this comment.
Pull request overview
This PR introduces tree-shaped configurations for Gorums to enable hop-by-hop fan-out and aggregation across large clusters, reducing the need for a single caller to maintain direct connections to every node.
Changes:
- Adds
Configuration.AsTree(opts)andTreeConfigurationAPIs (position/parent/children/subtree +Contextfor addressing root’s children). - Adds server-side tree registration and handler accessors via
Server.RegisterTreeandServerCtx.TreeChildren/TreeParent/TreePosition. - Adds unit/integration tests and documentation (including a working proto-based example under
internal/tests/tree/).
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tree.go | Implements TreeConfiguration, tree layout helpers, and Configuration.AsTree. |
| tree_test.go | Unit tests validating tree layout, parent/child/subtree computations, and ServerCtx accessors. |
| tree_integration_test.go | End-to-end tests for tree multicast and tree aggregation using real Systems. |
| server.go | Adds Server.tree field and RegisterTree API. |
| handler.go | Adds ServerCtx tree accessor methods. |
| internal/tests/tree/tree.proto | Defines TreeAggregator service to demonstrate tree fan-out and aggregation. |
| internal/tests/tree/tree.pb.go | Generated protobuf types for the tree test service. |
| internal/tests/tree/tree_gorums.pb.go | Generated Gorums bindings for the tree test service. |
| internal/tests/tree/tree_test.go | Integration-style tests exercising the generated tree service. |
| doc/user-guide.md | Adds “Tree-Structured Configurations” section and examples/diagram. |
Files not reviewed (2)
- internal/tests/tree/tree.pb.go: Language not supported
- internal/tests/tree/tree_gorums.pb.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add TreeConfiguration as a tree-shaped overlay on a flat Configuration so that fan-out and aggregation can be performed level-by-level instead of requiring the client to invoke RPCs to every node. - Configuration.AsTree(opts) builds a tree in breadth-first order; tree capacity is (bf^(depth+1) - 1) / (bf - 1). - TreeConfiguration exposes PositionOf, ParentOf, ChildrenOf, Subtree, and Context (addresses the root's direct children). - Server.RegisterTree binds a tree to a server so handlers can query their position via ServerCtx.TreeChildren, TreeParent, and TreePosition. - internal/tests/tree adds a TreeAggregator service exercising Broadcast (multicast) and Aggregate (quorum call) on a 7-node bf=2 depth=2 tree. - User guide gains a "Tree-Structured Configurations" section documenting the pattern with a sequence diagram. Note: The TreeConfiguration is currently constructed using the Configuration.AsTree() method based on the fully connected config.
3f2a0ff to
588c3c5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- internal/tests/tree/tree.pb.go: Language not supported
- internal/tests/tree/tree_gorums.pb.go: Language not supported
Comments suppressed due to low confidence (2)
doc/user-guide.md:2023
- This snippet ignores the error returned by AsTree (
sysTree, _ := ...). Since AsTree can fail on invalid options, empty config, or capacity overflow, the example should handle the error (and avoid registering a nil/partial tree).
for i, sys := range systems {
sysTree, _ := sys.OutboundConfig().AsTree(opts)
sys.RegisterService(nil, func(srv *gorums.Server) {
srv.RegisterTree(sysTree)
handler.go:139
- TreeParent also returns nil when the server's ID is not found in the registered TreeConfiguration (ParentOf returns nil for unknown IDs). The doc comment currently only mentions "root" and "no tree"; please document the not-in-tree case (or clarify that TreePosition can be used to distinguish it).
// TreeParent returns the parent [Node] of this server in the registered
// [TreeConfiguration], or nil if this server is the root or no tree is registered.
func (ctx *ServerCtx) TreeParent() *Node {
if ctx.srv == nil || ctx.srv.tree == nil {
return nil
}
return ctx.srv.tree.ParentOf(ctx.srv.myID)
}
These two methods YAGNI candidates; no clear use cases beyond diagnostics. We can add them back if we see a specific need.
cc0d22d to
8d3b25b
Compare
Adds tree-shaped configurations so that fan-out and aggregation across a large cluster can be performed hop-by-hop rather than requiring the caller to hold a direct connection to every node.
Configuration.AsTree(opts)builds aTreeConfigurationin breadth-first order.TreeConfigurationexposesParentOf,ChildrenOf,Subtree, andContext(the latter addresses the root's direct children — the caller acts as the external root).Server.RegisterTreebinds a tree to a server.ServerCtx.TreeChildrenandTreeParent.internal/tests/tree/adds aTreeAggregatorproto service that exercises both patterns on a 7-node bf=2, depth=2 tree:Broadcast(multicast): each internal node relays to its children before applying local logic; leaves apply only local logic.Aggregate(quorum call): each internal node releases the handler lock, calls its children, sums their totals with its own value, and returns; the caller sums the per-subtree totals.Usage sketch
Inside the server's handler (method):
Note: The
TreeConfigurationis currently constructed using theConfiguration.AsTree()method based on the fully connected config.