feat: Add mode schedules map #1071
Conversation
WalkthroughAdds planner.thrift include to orchestration.thrift and introduces an optional per-mode scheduling map field (modeSchedules) to ScheduleDeployRequest, ConfListItemResponse, and ConfGetResponse, using planner.Mode. No other fields changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OrchestrationService
participant Planner
Client->>OrchestrationService: ScheduleDeployRequest(modeSchedules)
OrchestrationService->>Planner: Validate modes (planner.Mode)
Planner-->>OrchestrationService: OK / details
OrchestrationService-->>Client: Conf*Response(modeSchedules)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tchow-zlai
requested review from
david-zlai,
nikhil-zlai,
piyush-zlai and
sean-zlai
![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 (2)
api/thrift/orchestration.thrift (2)
256-257: Add concise docs; consider payload implications for list APIs.Listing per-mode schedules may bloat responses; OK if intended. Long-term, consider aligning all “mode” fields to planner.Mode for consistency.
- 4: optional map<planner.Mode, string> modeSchedules + // Optional per-mode schedule map for this conf; may increase list payload size. + 4: optional map<planner.Mode, string> modeSchedules
273-274: Document field semantics; consider a request flag to control inclusion.ConfGetResponse might benefit from an includeSchedules flag in ConfGetRequest to avoid unnecessary data transfer.
- 5: optional map<planner.Mode, string> modeSchedules + // Optional per-mode schedule map for this conf. Consider a request flag to include/omit for performance. + 5: optional map<planner.Mode, string> modeSchedulesI can draft the ConfGetRequest toggle + downstream wiring if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/thrift/orchestration.thrift(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: analyzer_tests
- GitHub Check: groupby_tests
- GitHub Check: fetcher_tests
- GitHub Check: streaming_tests
- GitHub Check: spark_tests
- GitHub Check: join_tests
- GitHub Check: batch_tests
- GitHub Check: cloud_aws_tests
- GitHub Check: cloud_gcp_tests
- GitHub Check: service_commons_tests
- GitHub Check: service_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: online_tests
- GitHub Check: api_tests
- GitHub Check: aggregator_tests
- GitHub Check: flink_tests
- GitHub Check: python_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (1)
api/thrift/orchestration.thrift (1)
6-6: Include Verification Passed:planner.thriftDefinesMode& No Circular IncludeVerified that
api/thrift/planner.thriftexists, declaresenum Mode, and does not includeorchestration.thrift. No action needed.
| 4: optional map<planner.Mode, string> modeSchedules | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enum map keys can be problematic with JSON gateways; add field docs.
Thrift JSON stringifies enum keys (often numeric). Verify any JSON/HTTP boundary can handle this. Also document schedule format/timezone and null vs empty semantics.
Apply inline docs:
- 4: optional map<planner.Mode, string> modeSchedules
+ // Map from planner.Mode to schedule expression (e.g. cron). Clarify format/timezone and empty vs omitted semantics in docs.
+ 4: optional map<planner.Mode, string> modeSchedules🏁 Script executed:
#!/bin/bash
# Detect JSON protocol usage that may be impacted by enum map keys
rg -n 'TJSONProtocol|SimpleJSONProtocol|JsonProtocol' -S
# Find call-sites to ensure they populate/consume modeSchedules correctly
rg -n 'ScheduleDeployRequest|modeSchedules' -A2 -B2Length of output: 4508
Document modeSchedules field and JSON enum key behavior
- JSON protocols (
TSimpleJSONProtocol/TJSONProtocol) serialize enum keys as their numeric strings—ensure downstream JSON gateways accept numeric-string keys. - Add concise field docs for
modeSchedulesin all orchestration structs (lines 123, 256, 273): specify expected schedule format (e.g. cron), timezone (UTC), and semantics for omitted vs. empty map.
Suggested inline diff:
- 4: optional map<planner.Mode, string> modeSchedules
+ // Map from planner.Mode to cron schedule string (e.g. "0 0 * * *" UTC).
+ // Empty map = no modes scheduled; omitted = use default scheduling.
+ 4: optional map<planner.Mode, string> modeSchedules📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 4: optional map<planner.Mode, string> modeSchedules | |
| } | |
| // Map from planner.Mode to cron schedule string (e.g. "0 0 * * *" UTC). | |
| // Empty map = no modes scheduled; omitted = use default scheduling. | |
| 4: optional map<planner.Mode, string> modeSchedules | |
| } |
🤖 Prompt for AI Agents
In api/thrift/orchestration.thrift around lines 123, 256 and 273, the optional
map<planner.Mode,string> modeSchedules fields lack documentation and callers may
be surprised that TSimpleJSONProtocol/TJSONProtocol will serialize enum map keys
as numeric strings; update the three orchestration struct field comments to
document: the map keys are planner.Mode and when serialized to JSON may appear
as numeric string keys (gateway must accept numeric-string keys), the map values
are schedule strings in cron format (or clearly state accepted format), times
are interpreted in UTC, and semantics: omitted field = use default schedules, an
explicit empty map = disable all schedules, and invalid/empty strings are
treated as errors — add these concise notes inline for each occurrence.
| 1: optional string branch | ||
| 2: optional string confName | ||
| 3: optional string confHash | ||
| 4: optional map<planner.Mode, string> modeSchedules |
There was a problem hiding this comment.
By using planner.Mode I assume we'll continue to embrace Backfill and Deploy vs Offline and Online naming (I think both will always live on in various aspects).
I can do the mapping on my side, but there are cases where we pass the opposite (and know we had a CLI verb naming discussion a few days ago).
There was a problem hiding this comment.
I like discrete naming with an enum vs wildcard strings any day :)
Summary
Checklist
Summary by CodeRabbit