-
Notifications
You must be signed in to change notification settings - Fork 0
ME-4953: batch resources in server to device message #59
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
ME-4953: batch resources in server to device message #59
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new ServiceBatch message to device/device.proto and extends ServerToDeviceMessage’s oneof with a service_batch field, enabling batched delivery of repeated Service entries with batch metadata (total_services and batch_number). Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Device
Server->>Device: ServerToDeviceMessage{ service_batch(batch 1) }
Note right of Device: Receive services[], batch_number=1, total_services=N
loop Subsequent batches
Server->>Device: ServerToDeviceMessage{ service_batch(batch n) }
Note right of Device: Accumulate services until total_services reached
end
Server->>Device: ServerToDeviceMessage{ service_batch(batch final) }
Note right of Device: Verify total_services, process all services
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
device/device.proto
Outdated
|
|
||
| message ServiceBatch { | ||
| repeated Service services = 1; | ||
| bool is_first_batch = 2; // Optional: for progress tracking |
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.
Can we use standard pagination terminology e.g. “ServicePage” message, with a pagination object e.g. items, has_next_page, page_size, page_number etc.
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
device/device.proto (1)
39-39: Document service_batch usage, legacy fallback, and update handlingAdding
service_batch = 9to the existingoneofis wire-compatible, but we need to clarify its use and ensure any message-handling logic is updated:– In device/device.proto (around line 39), add an inline comment:
- ServiceBatch service_batch = 9; + // Batched services; only sent to agents advertising support for batching. + // Legacy agents continue receiving singular `service` messages. + ServiceBatch service_batch = 9;– In your public API/docs (e.g. RELEASE_NOTES, README), spell out:
• When the server will emitservice_batchvsservice
• How legacy agents will fall back (capability negotiation, version gating, or server-side downgrade)
• That interleavingserviceandservice_batchin a single session is only supported if explicitly handled.– Scan any application code that inspects the
ServerToDeviceMessageoneof (for example,switch messageCasein Swift/Kotlin or type-switch in Go) and confirm you handle the newserviceBatchbranch appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
device/device.pb.gois excluded by!**/*.pb.gogen/kotlin/border0/device/v1/ServerToDeviceMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/device/v1/ServiceBatchKt.ktis excluded by!**/gen/**gen/swift/device.pb.swiftis excluded by!**/gen/**
📒 Files selected for processing (1)
device/device.proto(2 hunks)
device/device.proto
Outdated
|
|
||
| message ServiceBatch { | ||
| repeated Service services = 1; | ||
| bool is_first_batch = 2; // Optional: for progress tracking |
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.
How/what-for do we use this field?
adrianosela
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.
See my comment regarding nomenclature. Otherwise lgtm
8402d37 to
41be327
Compare
41be327 to
421358e
Compare
Summary by CodeRabbit
New Features
Performance Improvements