[ME-3787] Move Connector Stats Message #43
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 buf (1.47.2)connector/connector.protoConfig file YAML parsing error: yaml: unmarshal errors: 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
connector/connector.proto (1)
35-35: Consider documenting the breaking change.The movement of
StatsMessagefrom responses to requests is a breaking change that requires coordination between client and server updates.Consider adding a comment above the field to document:
- When this change was made
- Why stats were moved from response to request
- Migration considerations for clients
+ // Stats field moved from ControlStreamResponse to ControlStreamRequest in PR #43 + // to transition from pull-based to push-based stats reporting border0.common.v1.StatsMessage stats = 17;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
connector/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
connector/connector.pb.swift(5 hunks)connector/connector.proto(1 hunks)
🔇 Additional comments (6)
connector/connector.proto (1)
35-35: Verify the architectural implications of moving stats to requests.
The addition of StatsMessage to ControlStreamRequest suggests a shift in the stats reporting architecture from a pull model (server requesting stats) to a push model (client sending stats). This is a breaking change that affects the API contract.
Consider the following implications:
- Clients must be updated to proactively send stats
- Servers must be updated to handle stats in requests instead of sending stats requests
- Migration strategy might be needed for backward compatibility
Let's verify the complete removal of stats from responses and check for any remaining references:
✅ Verification successful
Breaking change confirmed: Stats field moved from response to request
The verification confirms that:
- The
StatsMessagefield has been completely removed fromControlStreamResponse - The field is now only present in
ControlStreamRequestwith field number 17 - The change represents a complete architectural shift from pull to push model for stats reporting
This is indeed a breaking change that requires careful coordination between client and server updates. The removal from response and addition to request is clean, with no lingering references to stats in the response message.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete removal of stats handling from responses
# and check for any remaining references that might need updating
# Check for any remaining stats references in proto files
rg -i "stats.*message" --type proto
# Check for any response-related stats handling in the codebase
rg -i "stats.*response" --type proto
Length of output: 136
Script:
#!/bin/bash
# Let's try a different approach without relying on file type detection
# Check for any stats-related messages in proto files
fd '\.proto$' --exec rg -i 'stats.*message' {}
# Check for any response-related stats handling
fd '\.proto$' --exec rg -i 'stats.*response' {}
# Get the full context of the connector.proto file to verify the changes
fd 'connector\.proto$' --exec cat {}
Length of output: 7115
connector/connector.pb.swift (5)
195-201: LGTM: Stats property implementation follows the established pattern.
The implementation of the stats property correctly follows the getter/setter pattern used for other oneof fields in the struct.
221-221: LGTM: Stats case properly added to OneOf_RequestType enum.
The new case follows the established pattern and is correctly associated with the Border0_Common_V1_StatsMessage type.
1365-1377: LGTM: Stats decoding logic properly implemented.
The decoding implementation for the stats field follows the established pattern and correctly handles the oneof field semantics.
1449-1452: LGTM: Stats traversal logic properly implemented.
The traversal implementation for the stats field follows the established pattern and correctly handles the oneof field semantics.
195-201: Verify the complete migration of stats message.
While the addition of the stats field to the request type is properly implemented, let's verify that it has been completely removed from the response type as intended.
Also applies to: 221-221, 1365-1377, 1449-1452
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand both the stats message implementation and response type structures to verify the migration.
Stats message has been properly migrated
Based on the search results, I can confirm that:
- The stats message is correctly implemented in the request type (
Border0_V1_ControlStreamResponse.OneOf_RequestType) - There are no references to stats in the response type or its variants
- The complete enumeration of response types shows no stats-related fields
The migration appears to be complete and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to stats in the response messages
# This will help confirm the complete migration of the stats message
# Search for stats-related fields in response messages
rg -i 'stats.*response' --type swift
Length of output: 38
Script:
#!/bin/bash
# Let's try a broader search to find any stats-related code in the protobuf file
rg -A 5 -B 5 "stats" connector/connector.pb.swift
# Also search for response type definitions to understand the structure
rg -A 5 "Response" connector/connector.pb.swift
Length of output: 13929
[ME-3787] Move Connector Stats Message
Made a mistake in previous PR.
Summary by CodeRabbit