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

Handle start time & repeats #7

Merged
merged 9 commits into from
Mar 21, 2024
Merged

Conversation

artursouza
Copy link
Contributor

@artursouza artursouza commented Mar 16, 2024

  • Support startTime for delayed start of the cron (1h from now, for example)
  • Support for repeats (trigger 3 times only, for example)
  • Support custom logic to stop (delete) job, so app can stop the job with any logic they want
  • Removed unused struct

Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
@artursouza artursouza requested review from JoshVanL and cicoyle March 16, 2024 06:33
Copy link
Collaborator

@JoshVanL JoshVanL left a comment

Choose a reason for hiding this comment

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

I am quite concerned with the fragility of having a second-tick based approach for processing jobs on the queue. I am quite keen for us to move to a priority queue based approach as soon as possible.

assert.True(t, updated)
assert.Equal(t, 3, value)

time.Sleep(time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, use the k8s Clock so that it can be mocked in tests and can execute immediately. https://github.com/dapr/dapr/blob/ed56f692122d0afef7c79147f77eb283acb10467/pkg/placement/placement.go#L154

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't do it because the expiration is handled by etcd, so we need to wait for a real expiration in the database.

map<string, string> metadata = 3;
google.protobuf.Any payload = 4;
int64 startTimestamp = 3; // time that the schedule starts
int32 repeats = 4; // time total time the job should trigger
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to use 32? it is odd to not be consistent.

Suggested change
int32 repeats = 4; // time total time the job should trigger
int64 repeats = 4; // time total time the job should trigger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

int64 is a requirement for time. While the repeats is not expected to be that large, making int64 overkill. Even int32 is already "too much".

artursouza and others added 4 commits March 18, 2024 07:14
Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
@artursouza
Copy link
Contributor Author

I am quite concerned with the fragility of having a second-tick based approach for processing jobs on the queue. I am quite keen for us to move to a priority queue based approach as soon as possible.

We are not using second-tick, second is just the granularity chosen to keep the jobs. We will use priority queue to optimize the ticker since it is more efficient than re-sorting on every iteration.

@artursouza artursouza requested a review from JoshVanL March 18, 2024 17:22
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
@artursouza artursouza dismissed JoshVanL’s stale review March 21, 2024 20:15

Josh is not available due to an event. All applicable comments were addressed. Got another approval.

@artursouza artursouza merged commit c0f44b7 into diagridio:master Mar 21, 2024
1 check passed
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 this pull request may close these issues.

3 participants