Skip to content
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

It's impossible to send unset duration #2065

Closed
Duzhinsky opened this issue May 16, 2024 · 1 comment · Fixed by #2067
Closed

It's impossible to send unset duration #2065

Duzhinsky opened this issue May 16, 2024 · 1 comment · Fixed by #2067

Comments

@Duzhinsky
Copy link
Contributor

Duzhinsky commented May 16, 2024

Expected Behavior

When java.util.Duration is null in an options class like SchedulePolicy it is treated as an unset value in a protobuf message in some cases

Actual Behavior

null values are sent as zero duration. There was a TODO in the appropriate method about refactoring this behaviour, but was removed in #1950

public static com.google.protobuf.Duration toProtoDuration(Duration d) {
// TODO we should refactor an implicit conversion of empty values into ZERO and rename the
// current method into toJavaDurationSafe, toJavaDurationOrDefault or something like that
if (d == null) {
return Durations.ZERO;
}

A problem

I'm creating schedules using the following code:

...
var policy = SchedulePolicy.newBuilder()
                .setOverlap(ScheduleOverlapPolicy.SCHEDULE_OVERLAP_POLICY_BUFFER_ONE)
                .build();
var schedule Schedule.newBuilder()
                .setAction(actionStartWorkflow)
                .setSpec(spec)
                .setPolicy(policy)
                .build();

Which causes sending 0 in the catchupWindow property of SchedulePolicy. Hence, server side treats it as 0 and uses MinCatchupWindow (=10 sec) instead of expected DefaultCatchupWindow (=1yr)

https://github.com/temporalio/temporal/blob/aca0d911a908b2584f4cffcc73425c5119dbe722/service/worker/scheduler/workflow.go#L750-L759

Steps to Reproduce the Problem

  1. Create a schedule using the code above
  2. Check its catchupWindow
@Quinn-With-Two-Ns
Copy link
Contributor

Most of time the Temporal server treats zero and unset durations the same, unfortunately it seems like schedules makes a distinction. Probably need to add conversion functions for that preserve null for time conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants