WIP -- create backfill tracker thrift apis #224
Conversation
WalkthroughThe pull request introduces a new Thrift file Changes
Sequence DiagramsequenceDiagram
participant Client
participant HubVerticle
participant JobTracker
Client->>HubVerticle: Request Job by Type
HubVerticle->>JobTracker: JobTrackerRequest
JobTracker-->>HubVerticle: JobTrackerResponse
HubVerticle-->>Client: Response with Job Details
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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 (
|
api/thrift/hub.thrift
Outdated
|
|
||
| // Job corresponds to top-level runs for the Entity, can be either user-run adhoc (via CLI/python API) or scheduled | ||
| // We should move this to orchestration.thrift if we keep it | ||
| struct Job { |
There was a problem hiding this comment.
Still some question here around how to handle overlapping jobs for a given day. Probably just take the latest for each day is good enough for now is my current thought.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/thrift/hub.thrift (1)
1-8: Remove extra blank line.include "orchestration.thrift" -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/thrift/hub.thrift(1 hunks)api/thrift/orchestration.thrift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/thrift/orchestration.thrift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (4)
api/thrift/hub.thrift (4)
30-35: LGTM! Clean and well-structured response type.
37-41: LGTM! Comprehensive direction options.
62-65: LGTM! Clear mode definitions.
50-60: 🛠️ Refactor suggestionAddress structural concerns.
- Decide on struct location (see comment)
- Use i64 for dates
- Document null handling for endTime
- 2: required string startDate - 3: required string endDate + 2: required i64 startTimestamp + 3: required i64 endTimestamp
| struct JobTrackerRequest { | ||
| 1: required orchestration.NodeKey node | ||
| 2: required Direction direction | ||
| 3: optional string startDate // Do we need a way to specify a range, in case of very large jobs? | ||
| 4: optional string endDate | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use i64 timestamps instead of string dates.
String dates are error-prone. Consider:
- 3: optional string startDate
- 4: optional string endDate
+ 3: optional i64 startTimestamp
+ 4: optional i64 endTimestamp📝 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.
| struct JobTrackerRequest { | |
| 1: required orchestration.NodeKey node | |
| 2: required Direction direction | |
| 3: optional string startDate // Do we need a way to specify a range, in case of very large jobs? | |
| 4: optional string endDate | |
| } | |
| struct JobTrackerRequest { | |
| 1: required orchestration.NodeKey node | |
| 2: required Direction direction | |
| 3: optional i64 startTimestamp | |
| 4: optional i64 endTimestamp | |
| } |
api/thrift/hub.thrift
Outdated
| struct StepRange { | ||
| 1: required string start | ||
| 2: required string end | ||
| 3: required Status status | ||
| 4: required string logLocation // Or maybe download logs is a different API? | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine date handling and clarify log strategy.
- Use timestamps for consistency
- Resolve the log API question in comment
- 1: required string start
- 2: required string end
+ 1: required i64 startTimestamp
+ 2: required i64 endTimestamp📝 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.
| struct StepRange { | |
| 1: required string start | |
| 2: required string end | |
| 3: required Status status | |
| 4: required string logLocation // Or maybe download logs is a different API? | |
| } | |
| struct StepRange { | |
| 1: required i64 startTimestamp | |
| 2: required i64 endTimestamp | |
| 3: required Status status | |
| 4: required string logLocation // Or maybe download logs is a different API? | |
| } |
varant-zlai
force-pushed
the
vz--create_backfill_tracker_thrift_apis
branch
from
ad976b7 to
ea48018
Compare
api/thrift/hub.thrift
Outdated
| 1: optional string name | ||
| 2: optional string type // physical type (limited to backfill or batch upload) | ||
| 3: required Direction direction | ||
| 4: optional string startDate // Do we need a way to specify a range, in case of very large jobs? |
There was a problem hiding this comment.
start these from 10 - so we can insert non time related options in the request later.
api/thrift/hub.thrift
Outdated
| */ | ||
| struct JobTrackerRequest { | ||
| 1: optional string name | ||
| 2: optional string type // physical type (limited to backfill or batch upload) |
api/thrift/hub.thrift
Outdated
| 3: required Status status | ||
| 4: required string logLocation // Or maybe download logs is a different API? |
There was a problem hiding this comment.
Ideally we have a struct
NodeInfo {
1: optional string name
2: optional string version
3: optional string physicalType
10: optional list<TaskInfo> tasks // very old job infos might be be pruned
11: optional List<DateRange> availablePartitions // specific to batch nodes
}
// general purpose
TaskInfo {
1: Status status
2: string logPath
3: string trackerUrl
4: TaskArgs args
5: DateRange dateRange // specific to batch nodes
// time information - useful for gantt / waterfall view
10: long submittedTs
11: long startedTs
12: long finishedTs
20: string user
21: string team
// utilization information
30: TaskResources allocatedResources
31: TaskResources utilizedResources
}
TaskArgs {
1: list<string> args
2: map<string, string> env
}
TaskResources {
1: optional long vcoreSeconds
2: optional long megaByteSeconds
3: optional long cumulativeDiskWriteBytes
4: optional long cumulativeDiskReadBytes
// networking, gpu etc
}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
api/thrift/hub.thrift (5)
25-25: 🛠️ Refactor suggestionConvert type to enum.
Define an enum for job types (backfill/batch upload) instead of using string.
27-28: 🛠️ Refactor suggestionUse i64 timestamps.
String dates are error-prone. Use timestamps.
- 4: optional string startDate - 5: optional string endDate + 4: optional i64 startTimestamp + 5: optional i64 endTimestamp
45-46: 🛠️ Refactor suggestionUse i64 timestamps in StepRange.
Maintain consistency with timestamp usage.
- 1: required string startDate - 2: required string endDate + 1: required i64 startTimestamp + 2: required i64 endTimestamp
55-56: 🛠️ Refactor suggestionUse i64 timestamps in Job struct.
Maintain consistency with timestamp usage.
- 2: required string startDate - 3: required string endDate + 2: required i64 startTimestamp + 3: required i64 endTimestamp
74-75: 🛠️ Refactor suggestionAdd missing comma after UPSTREAM_FAILED.
- UPSTREAM_FAILED = 5, + UPSTREAM_FAILED = 5,
🧹 Nitpick comments (2)
api/thrift/hub.thrift (2)
51-53: Move Job struct to orchestration.thrift.Consider moving as suggested in the comment for better organization.
63-66: Start Mode enum values from 10.Follow same convention as Direction enum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/thrift/hub.thrift(1 hunks)hub/src/main/java/ai/chronon/hub/HubVerticle.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hub/src/main/java/ai/chronon/hub/HubVerticle.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (4)
api/thrift/hub.thrift (4)
1-6: LGTM! Proper namespace setup and includes.
31-36: LGTM! Clear and well-structured response type.
38-42: Start enum values from 10.Per team convention, start from 10 to allow for future non-directional options.
enum Direction { - UPSTREAM = 0, - DOWNSTREAM = 1, - BOTH = 2 + UPSTREAM = 10, + DOWNSTREAM = 11, + BOTH = 12 }
48-48: Clarify log API strategy.Decide whether log retrieval should be a separate API endpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/thrift/hub.thrift (1)
85-88: 🛠️ Refactor suggestionUse i64 timestamps instead of string dates.
String dates are error-prone and harder to validate.
struct DateRange { - 1: string startDate - 2: string endDate + 1: i64 startTimestamp + 2: i64 endTimestamp }
🧹 Nitpick comments (4)
api/thrift/hub.thrift (4)
65-83: Consider making timestamps optional.Not all tasks will have startedTs and finishedTs immediately.
- 11: required i64 startedTs - 12: required i64 finishedTs + 11: optional i64 startedTs + 12: optional i64 finishedTs
46-46: Document the task ordering strategy.Add a comment clarifying how overlapping tasks are handled.
- 1: required list<TaskInfo> tasks // Date ranges can overlap for tasks (reruns, retries etc). Need to render latest per day. + 1: required list<TaskInfo> tasks // Tasks are ordered by submittedTs desc, with latest task per day shown in UI
90-93: Make args and env required fields.TaskArgs fields should be non-optional to ensure complete task information.
struct TaskArgs { - 1: list<string> args - 2: map<string, string> env + 1: required list<string> args + 2: required map<string, string> env }
119-123: Consider adding status field to Submission.Status would help track submission progress.
struct Submission { + 22: optional Status status }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/thrift/hub.thrift(1 hunks)api/thrift/orchestration.thrift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/thrift/orchestration.thrift
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: no_spark_scala_tests
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (2)
api/thrift/hub.thrift (2)
1-6: LGTM!Namespaces and includes are properly defined.
108-116:⚠️ Potential issueAdd missing comma after UPSTREAM_FAILED.
RUNNING = 2, SUCCESS = 3, FAILED = 4, - UPSTREAM_FAILED = 5 + UPSTREAM_FAILED = 5, UPSTREAM_MISSING = 6Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/thrift/hub.thrift (1)
90-90: Fix indentation.Align closing brace with struct definition.
- } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/thrift/hub.thrift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (2)
api/thrift/hub.thrift (2)
76-77: Use i64 timestamps.Convert date strings to timestamps for consistency and reliability.
- 1: string startDate - 2: string endDate + 1: i64 startTimestamp + 2: i64 endTimestamp
104-105: Add missing comma.Fix syntax error in enum definition.
UPSTREAM_FAILED = 5, UPSTREAM_MISSING = 6
api/thrift/hub.thrift
Outdated
| 10: required i64 submittedTs | ||
| 11: required i64 startedTs | ||
| 12: required i64 finishedTs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make timestamps optional.
startedTs and finishedTs won't be available for pending tasks.
- 11: required i64 startedTs
- 12: required i64 finishedTs
+ 11: optional i64 startedTs
+ 12: optional i64 finishedTs📝 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.
| 10: required i64 submittedTs | |
| 11: required i64 startedTs | |
| 12: required i64 finishedTs | |
| 10: required i64 submittedTs | |
| 11: optional i64 startedTs | |
| 12: optional i64 finishedTs |
api/thrift/hub.thrift
Outdated
| // If the page was accessed via a "submission" link, then we also render the submission range | ||
| struct LineageRequest { | ||
| 1: required string name | ||
| 2: required string type // physical type (limited to backfill or batch upload) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert type to enum.
Replace string type with enum to prevent errors and improve type safety.
- 2: required string type // physical type (limited to backfill or batch upload)
+ 2: required PhysicalType type
+enum PhysicalType {
+ BACKFILL = 0,
+ BATCH_UPLOAD = 1
+}Committable suggestion skipped: line range outside the PR's diff.
| 2: required orchestration.NodeKey mainNode // Same as the node in the LineageRequest | ||
| } | ||
|
|
||
| struct JobTrackerRequest { |
There was a problem hiding this comment.
| struct JobTrackerRequest { | |
| struct JobTrackerRequest { |
Should we just call this a Submission? Then the vocabulary becomes - tracking a submission.
There was a problem hiding this comment.
Discussed offline, sometimes there's no associated submission
api/thrift/hub.thrift
Outdated
| 1: required Status status | ||
| 2: required string logPath | ||
| 3: required string trackerUrl | ||
| 4: required TaskArgs args | ||
| 5: required DateRange dateRange // specific to batch nodes | ||
|
|
||
| // time information - useful for gantt / waterfall view | ||
| 10: required i64 submittedTs | ||
| 11: required i64 startedTs | ||
| 12: required i64 finishedTs | ||
|
|
||
| 20: required string user | ||
| 21: required string team | ||
|
|
||
| // utilization information | ||
| 30: required TaskResources allocatedResources | ||
| 31: required TaskResources utilizedResources | ||
| } |
There was a problem hiding this comment.
All these should be optional instead of required
| struct DateRange { | ||
| 1: string startDate | ||
| 2: string endDate | ||
| } | ||
|
|
||
| struct TaskArgs { | ||
| 1: list<string> args | ||
| 2: map<string, string> env | ||
| } |
There was a problem hiding this comment.
add optional to all fields - it is a convention in thrift to do so - to make future changes easy to ship.
api/thrift/hub.thrift
Outdated
| 1: required orchestration.NodeKey node | ||
| 10: required i64 submittedTs | ||
| 20: optional i64 finishedTs | ||
| 21: optional DateRange dateRange |
There was a problem hiding this comment.
optional everywhere please.
|
|
||
| def handle(req: JobTrackerRequest): JobTrackerResponse = { | ||
| val result = new JobTrackerResponse() | ||
| result |
api/thrift/hub.thrift
Outdated
| 10: required i64 submittedTs | ||
| 20: optional i64 finishedTs |
There was a problem hiding this comment.
I would split these out into submissionInfo - at the time of submission we won't finishedTs etc.
varant-zlai
force-pushed
the
vz--create_backfill_tracker_thrift_apis
branch
from
b98c268 to
3a53226
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/thrift/hub.thrift (2)
79-80: 🛠️ Refactor suggestionUse i64 timestamps instead of string dates.
- 1: string startDate - 2: string endDate + 1: i64 startTimestamp + 2: i64 endTimestamp
101-109:⚠️ Potential issueAdd missing comma after UPSTREAM_FAILED.
RUNNING = 2, SUCCESS = 3, FAILED = 4, - UPSTREAM_FAILED = 5 + UPSTREAM_FAILED = 5, UPSTREAM_MISSING = 6
🧹 Nitpick comments (2)
api/thrift/hub.thrift (2)
39-39: Document task deduplication strategy.Add comment explaining how latest task per day is selected for overlapping ranges.
111-116: Consider adding status field to Submission.Track submission state for better user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/thrift/hub.thrift(1 hunks)hub/src/main/scala/ai/chronon/hub/handlers/JobTracker.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hub/src/main/scala/ai/chronon/hub/handlers/JobTracker.scala
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (2)
api/thrift/hub.thrift (2)
58-76: TaskInfo structure looks good.Well-structured with optional fields and comprehensive metadata.
19-22: 🛠️ Refactor suggestionConvert type to enum for type safety.
- 2: optional string type // physical type (limited to backfill or batch upload) + 2: optional PhysicalType type +enum PhysicalType { + BACKFILL = 0, + BATCH_UPLOAD = 1 +}Likely invalid or redundant comment.
| 1: optional string name | ||
| 2: optional string type | ||
| 3: optional string branch | ||
| 10: optional DateRange dateRange // We may not need to use this, but in case it helps with page load times |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make type field consistent with LineageRequest.
Use the same PhysicalType enum here.
varant-zlai
force-pushed
the
vz--create_backfill_tracker_thrift_apis
branch
from
3a53226 to
a591b53
Compare
ken-zlai
left a comment
ken-zlai
left a comment
There was a problem hiding this comment.
adding a few notes from our discussion
api/thrift/hub.thrift
Outdated
| SUCCESS = 3, | ||
| FAILED = 4, | ||
| UPSTREAM_FAILED = 5, | ||
| UPSTREAM_MISSING = 6 |
| 31: optional TaskResources utilizedResources | ||
| } | ||
|
|
||
| struct DateRange { |
There was a problem hiding this comment.
Format is yyyy-MM-dd. In the UI we treat this as it comes in, no adjusting for time zones.
| BOTH = 2 | ||
| } | ||
|
|
||
| struct TaskInfo { |
There was a problem hiding this comment.
Anything with Ts is an epoch time in MS
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
varant-zlai
force-pushed
the
vz--create_backfill_tracker_thrift_apis
branch
from
a591b53 to
9dc144a
Compare
## Summary Input/out schema for the Job tracker and lineage API on ZiplineHub. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a comprehensive job tracking system with support for tracking job execution, status, and metadata. - Added a new API endpoint for handling job type requests. - Defined structures for job tracking requests and responses, including job metadata. - Introduced enumerations for job processing direction, execution modes, and statuses. - Added a new handler for processing job tracking requests. - **Bug Fixes** - Corrected a typographical error in a comment within the orchestration Thrift file. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary Input/out schema for the Job tracker and lineage API on ZiplineHub. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a comprehensive job tracking system with support for tracking job execution, status, and metadata. - Added a new API endpoint for handling job type requests. - Defined structures for job tracking requests and responses, including job metadata. - Introduced enumerations for job processing direction, execution modes, and statuses. - Added a new handler for processing job tracking requests. - **Bug Fixes** - Corrected a typographical error in a comment within the orchestration Thrift file. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary Input/out schema for the Job tracker and lineage API on ZiplineHub. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a comprehensive job tracking system with support for tracking job execution, status, and metadata. - Added a new API endpoint for handling job type requests. - Defined structures for job tracking requests and responses, including job metadata. - Introduced enumerations for job processing direction, execution modes, and statuses. - Added a new handler for processing job tracking requests. - **Bug Fixes** - Corrected a typographical error in a comment within the orchestration Thrift file. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary Input/out schema for the Job tracker and lineage API on ZiplineHub. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a comprehensive job tracking system with support for tracking job execution, status, and metadata. - Added a new API endpoint for handling job type requests. - Defined structures for job tracking requests and responses, including job metadata. - Introduced enumerations for job processing direction, execution modes, and statuses. - Added a new handler for processing job tracking requests. - **Bug Fixes** - Corrected a typographical error in a comment within the orchestration Thrift file. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary Input/out schema for the Job traour clientser and lineage API on ZiplineHub. ## Cheour clientslist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a comprehensive job traour clientsing system with support for traour clientsing job execution, status, and metadata. - Added a new API endpoint for handling job type requests. - Defined structures for job traour clientsing requests and responses, including job metadata. - Introduced enumerations for job processing direction, execution modes, and statuses. - Added a new handler for processing job traour clientsing requests. - **Bug Fixes** - Corrected a typographical error in a comment within the orchestration Thrift file. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
WIP input/out schema for the Job tracker API on ZiplineHub
Checklist
Summary by CodeRabbit
New Features
Bug Fixes