feat(schedules): implement schedule gRPC wiring and ScheduleClientImpl#1069
Conversation
abhishekj720
requested review from
demirkayaender,
dkrotx,
natemort and
shijiesheng
as code owners
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
abhishekj720
force-pushed
the
feat/schedule-impl
branch
2 times, most recently
from
ce7ffd0 to
b0984d0
Compare
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
abhishekj720
force-pushed
the
feat/schedule-impl
branch
from
b0984d0 to
9f5538e
Compare
Code Review 👍 Approved with suggestions 3 resolved / 5 findingsImplements gRPC wiring and ScheduleClientImpl to enable schedule service interactions, resolving previous issues with thread pool usage and backfill mapping. Ensure async error handling is updated to propagate typed exceptions instead of generic RuntimeExceptions and replace the unbounded cached thread pool. 💡 Quality: Async failures wrapped in generic RuntimeException lose typed errors📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:70-74 📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:99-103 📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:164-168 Every async body in 💡 Performance: Unbounded cached thread pool for blocking schedule calls📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:64-70 The newly introduced Consider a bounded pool (e.g. ✅ 3 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
abhishekj720
changed the title
Code Review ✅ Approved 5 resolved / 5 findingsImplements gRPC wiring and ScheduleClientImpl for schedule services, successfully addressing issues with default backfill NPEs, dropped overlap policies, and incorrect thread pool usage. ✅ 5 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
✅ Quality: Async failures wrapped in generic RuntimeException lose typed errors
✅ Performance: Unbounded cached thread pool for blocking schedule calls
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Adds com.uber.cadence.client.schedule — public Java API types for the Schedules feature using Instant/Duration instead of raw nanoseconds. Types added: - ScheduleSpec — cron expression, start/end time, jitter - ScheduleAction — wraps StartWorkflowAction for triggered workflows - SchedulePolicies — overlap, catch-up, and pause-on-failure behavior - ScheduleState — pause state (paused flag, reason, time, actor) - ScheduleInfo — runtime stats (runs, backfills, missed/skipped) - ScheduleDescription — full describe response; mutable for update calls - ScheduleListEntry — lightweight list entry from visibility store - ListSchedulesResult — paginated list response with next-page token - ScheduleOverlapPolicy — SKIP_NEW, BUFFER, CONCURRENT, CANCEL_PREVIOUS, TERMINATE_PREVIOUS - ScheduleCatchUpPolicy — SKIP, ONE, ALL
…tract types - Change ScheduleBackfill start/end time from ZonedDateTime to Instant; the Thrift wire format stores epoch nanoseconds (timezone-unaware), so Instant is the correct representation and is consistent with ScheduleSpec - Make ScheduleDescription fully immutable: remove all setters (setSpec, setAction, setPolicies, setMemo, setSearchAttributes), make fields final, and update class Javadoc to document the correct read-modify-write pattern via UpdateScheduleRequest - Fix stale WorkflowClient Javadoc @link references across all client/schedule types (ScheduleDescription, ScheduleInfo, ScheduleListEntry, ScheduleState, ScheduleOverlapPolicy, SchedulePolicies, ListSchedulesResult); all schedule methods live on ScheduleClient, not WorkflowClient - Soften getPauseReason() Javadoc: "Non-null only when isPaused() is true" changed to "null when not paused" to avoid implied contract beyond what the field stores - Remove ScheduleTypesTest tests that exercised removed setters (scheduleDescription_setters, setSpec_rejectsNull, setAction_rejectsNull, setPolicies_rejectsNull); rewrite notEqualOnDifferentSpec and toString tests to construct ScheduleDescription directly without setters
…eduleDescription immutability ScheduleBackfill is an immutable value type but was missing equals() and hashCode(), unlike every other schedule model type. Add value-based implementations over startTime, endTime, and overlapPolicy. ScheduleDescription.getMemo() and getSearchAttributes() were returning the caller-supplied map by reference, allowing post-construction mutation and defeating the immutability established by making the fields final. Defensive-copy and null-normalize both maps in the constructor (null -> emptyMap, non-null -> unmodifiableMap of a new HashMap), matching the pattern used by ScheduleInfo and ListSchedulesResult. Getters are now guaranteed non-null. Add tests covering the new equals/hashCode contract for ScheduleBackfill and the null-normalization, unmodifiability, and non-aliasing guarantees for ScheduleDescription maps.
describeSchedule now returns ScheduleDescription instead of DescribeScheduleResponse, and listSchedules now returns ListSchedulesResult instead of ListSchedulesResponse. The client/schedule/ model layer was already documented and built to be the public API surface; returning Thrift types was inconsistent with that design. The implementation layer (PR4) will perform the Thrift-to-model mapping.
…dulesResponse Remove the custom ListSchedulesResult and ScheduleListEntry types; listSchedules now returns the generated ListSchedulesResponse directly.
- Add ScheduleAPIGrpc blocking/future stubs to IGrpcServiceStubs and GrpcServiceStubs - Add schedule enum mappers (OverlapPolicy, CatchUpPolicy) to EnumMapper - Add schedule type mappers (ScheduleSpec, ScheduleAction, SchedulePolicies, ScheduleState, ScheduleInfo, ScheduleListEntry) to TypeMapper - Add schedule request mappers (create/describe/update/delete/pause/unpause/ backfill/list) to RequestMapper - Add schedule response mappers (create/describe/list) to ResponseMapper - Implement all 8 schedule methods in WorkflowServiceGrpc using ScheduleAPIGrpc - Add ScheduleClientImpl implementing ScheduleClient, mapping Thrift responses to client model types (Instant, Duration, enums) - Wire WorkflowClientInternal.scheduleClient() to return ScheduleClientImpl
…hedules Remove the toListSchedulesResult mapping and unused ListSchedulesResult/ ScheduleListEntry imports; the service already returns ListSchedulesResponse.
…l overlap policy Move all blocking gRPC calls off ForkJoinPool.commonPool() onto a dedicated daemon thread pool. Warn when ScheduleBackfill.overlapPolicy is set, as the current proto contract has no overlap_policy field on BackfillScheduleRequest and the value is silently dropped before reaching the server.
…RuntimeException Callers now unwrap one layer (CompletionException -> CadenceError) instead of two (CompletionException -> RuntimeException -> CadenceError), making typed error handling on EntityNotExistsError and similar exceptions straightforward.
…quest The proto BackfillScheduleRequest already has overlap_policy (field 5) and backfill_id (field 6); they were simply missing from the RequestMapper. Wire both through and restore setting overlapPolicy in ScheduleClientImpl.
abhishekj720
force-pushed
the
feat/schedule-impl
branch
from
542718a to
161346e
Compare
…ods to IWorkflowService Default implementations wrap the blocking counterparts, consistent with the existing ScheduleClientImpl executor pattern. Callers that need a CompletableFuture handle can use these directly without reimplementing the wrapping.
…esult references from ScheduleTypesTest The ListSchedulesResult wrapper was removed in favour of ListSchedulesResponse in a prior commit in this PR. The four listSchedulesResult_* tests that still reference the removed type are deleted. A merge conflict during rebase also caused the seven scheduleBackfill_* test methods to appear twice; the second copy is removed.
abhishekj720
changed the base branch from
feat/schedule-service-contract
to
master
Code Review 👍 Approved with suggestions 7 resolved / 8 findingsImplements end-to-end gRPC wiring for schedule APIs, resolving issues with thread pool usage, mapping logic, and test stability. Consider explicitly configuring the async schedule dispatcher to avoid potential contention on the common ForkJoinPool. 💡 Performance: Async schedule defaults dispatch blocking gRPC on commonPool📄 src/main/java/com/uber/cadence/serviceclient/IWorkflowService.java:905-919 The eight new Suggested fix: either provide an overload that accepts an ✅ 7 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
✅ Bug: Duplicate test methods break compilation
✅ Bug: Tests reference nonexistent ListSchedulesResult type
...and 2 more resolved from earlier reviews 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
These defaults dispatched blocking gRPC calls on ForkJoinPool.commonPool, the same anti-pattern ScheduleClientImpl avoids with its dedicated BLOCKING_EXECUTOR. Async dispatch belongs in ScheduleClientImpl, not at the service interface layer.
Code Review ✅ Approved 8 resolved / 8 findingsImplements full gRPC wiring and client-side logic for the schedule API, resolving blocking I/O and mapping issues found during implementation. All pending issues addressed, enabling end-to-end functionality for schedule operations. ✅ 8 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
✅ Bug: Duplicate test methods break compilation
✅ Bug: Tests reference nonexistent ListSchedulesResult type
...and 3 more resolved from earlier reviews OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
…re interface Change all 8 schedule methods on IWorkflowService from blocking (throws CadenceError) to CompletableFuture<T>. Implement via scheduleFutureStub() in WorkflowServiceGrpc using a toCompletableFuture helper that bridges ListenableFuture through Guava FutureCallback — no extra thread pool needed. Drop BLOCKING_EXECUTOR from ScheduleClientImpl entirely; each method now delegates directly to service.Foo(request). backfillSchedule fires all requests concurrently via CompletableFuture.allOf(). Update stub implementations in IWorkflowServiceBase, TestWorkflowService, TestWorkflowEnvironmentInternal, and TestActivityEnvironmentInternal to match the new return types.
…eptionally RequestMapper/EnumMapper calls are evaluated before toCompletableFuture is entered, so any IllegalArgumentException (e.g. unrecognized enum values) previously escaped synchronously instead of completing the returned future. Wrap all 8 schedule methods in try/catch and use a failedFuture helper to route mapping failures through the future contract.
Code Review 👍 Approved with suggestions 10 resolved / 11 findingsImplements end-to-end gRPC wiring for the schedule API and introduces a dedicated executor for asynchronous operations, addressing multiple mapping and concurrency issues. Ensure response mapping is offloaded from network threads to prevent blocking gRPC execution. 💡 Performance: directExecutor runs response mapping on gRPC network threads📄 src/main/java/com/uber/cadence/serviceclient/WorkflowServiceGrpc.java:36 📄 src/main/java/com/uber/cadence/serviceclient/WorkflowServiceGrpc.java:1450-1464 📄 src/main/java/com/uber/cadence/internal/sync/ScheduleClientImpl.java:78
✅ 10 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
✅ Bug: Duplicate test methods break compilation
✅ Bug: Tests reference nonexistent ListSchedulesResult type
...and 5 more resolved from earlier reviews 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Code Review ✅ Approved 11 resolved / 11 findingsImplements end-to-end gRPC wiring for the schedule API and introduces a dedicated executor for asynchronous client operations, successfully resolving all mapping, thread pool, and test infrastructure findings. ✅ 11 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
✅ Bug: Duplicate test methods break compilation
✅ Bug: Tests reference nonexistent ListSchedulesResult type
...and 6 more resolved from earlier reviews OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
1 similar comment
Code Review ✅ Approved 11 resolved / 11 findingsImplements end-to-end gRPC wiring for the schedule API and introduces a dedicated executor for asynchronous client operations, successfully resolving all mapping, thread pool, and test infrastructure findings. ✅ 11 resolved✅ Bug: backfillSchedule NPEs on default-constructor ScheduleBackfill precision
✅ Bug: Backfill overlap policy is silently dropped in proto mapping
✅ Performance: Blocking gRPC calls dispatched on ForkJoinPool.commonPool()
✅ Bug: Duplicate test methods break compilation
✅ Bug: Tests reference nonexistent ListSchedulesResult type
...and 6 more resolved from earlier reviews OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
What changed
Implements the gRPC wiring and client-side logic for the schedule API introduced in #1067.
gRPC stubs (
IGrpcServiceStubs,GrpcServiceStubs): addsScheduleAPIGrpcblocking stub, wired to the existing channel lifecycle.Compatibility layer (Thrift↔proto):
EnumMapper: bidirectionalscheduleOverlapPolicy,scheduleCatchUpPolicyTypeMapper: bidirectional mapping forscheduleSpec,scheduleAction,schedulePolicies,schedulePauseInfo,scheduleState,backfillInfo,scheduleInfo,scheduleListEntryRequestMapper: all 8 schedule request types (Thrift → proto), includingoverlap_policyandbackfill_idonBackfillScheduleRequestResponseMapper: create/describe/list/update/delete/pause/unpause/backfill schedule responses (proto → Thrift)WorkflowServiceGrpc: implements all 8 schedule methods by callingscheduleBlockingStub(), mapping throughRequestMapper/ResponseMapper.ScheduleClientImpl: new class implementingScheduleClient. Wraps synchronousIWorkflowServicecalls inCompletableFutureon a dedicated daemon thread pool (keeps blocking gRPC I/O offForkJoinPool.commonPool()). Maps Thrift gen types to clean client model types (Instant,Duration, client-side enums). Failures surface asCompletionExceptionwrapping the original typed exception so callers can react toEntityNotExistsErroretc. without double-unwrapping.WorkflowClientInternal:scheduleClient()now returnsScheduleClientImpl.Why
Previous state: all 8 schedule methods on
IWorkflowServiceandScheduleClientthrewUnsupportedOperationException. This PR makes them functional end-to-end against a Cadence server with the schedule API enabled.How it was tested
./gradlew compileJava -x generateProtoclean build./gradlew googleJavaFormat -x generateProtoformat cleanScheduleTypesTestpassDependencies
Stacked on
feat/schedule-service-contract(#1067).