Skip to content

feat: Add mode schedules map #1071

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.

Sign up for GitHub

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

Merged
tchow-zlai merged 1 commit into from
Aug 13, 2025
Merged

feat: Add mode schedules map #1071

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/thrift/orchestration.thrift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ namespace java ai.chronon.orchestration

include "common.thrift"
include "api.thrift"
include "planner.thrift"

// This has to be 0-indexed for Java usage
enum ConfType {
Expand Down Expand Up @@ -119,6 +120,7 @@ struct ScheduleDeployRequest {
1: optional string branch
2: optional string confName
3: optional string confHash
4: optional map<planner.Mode, string> modeSchedules
Copy link
Contributor

@sean-zlai sean-zlai Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@sean-zlai sean-zlai Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like discrete naming with an enum vs wildcard strings any day :)

}
Comment on lines +123 to 124
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 -B2

Length 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 modeSchedules in 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.

Suggested change
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.


struct ScheduleDeployResponse {
Expand Down Expand Up @@ -251,6 +253,7 @@ struct ConfListItemResponse {
1: optional string confName
2: optional ConfType confType
3: optional string confHash
4: optional map<planner.Mode, string> modeSchedules
}

struct ConfListResponse {
Expand All @@ -267,6 +270,7 @@ struct ConfGetResponse {
2: optional string confHash
3: optional ConfType confType
4: optional LogicalNode confContents
5: optional map<planner.Mode, string> modeSchedules
}

// ====================== End of Orchestration Service API Types ======================
Expand Down