Skip to content

Add the Jobs SDK #1255

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

Merged
merged 34 commits into from
Apr 16, 2025
Merged

Add the Jobs SDK #1255

merged 34 commits into from
Apr 16, 2025

Conversation

siri-varma
Copy link
Contributor

@siri-varma siri-varma commented Mar 12, 2025

PR Overview: Jobs SDK Integration

This PR introduces the Jobs SDK with three main APIs: Schedule, Get, and Delete Job. The implementation is divided into the following categories:

  • Core Logic
  • Models
  • Tests (Unit and Integration)

Core Logic

The core logic is implemented in two key classes, as detailed below:

Class Description Details
CronExpressionBuilder Builds Cron expressions compliant with Dapr sidecar expectations. - Supports valid Cron syntax (*, /, and other ranges).
- References: Jobs API and .NET SDK.
DaprJobsClient Manages the ScheduleJob, Get, and DeleteJob APIs. Handles validations and async calls. - Schedules, retrieves, and deletes jobs.
- Validates inputs and performs async operations for job management.

API Contracts

The following are the method signatures for the key APIs:

1. Schedule Job

This API schedules a job based on the provided request.

public Mono<Void> scheduleJob(ScheduleJobRequest createJobRequest);
  • Input: createJobRequest — Contains the necessary parameters to schedule the job.
  • Output: Returns a Mono<Void> to signify the completion of the scheduling operation.

2. Get Job

This API retrieves a job's status or details based on the given request.

public Mono<GetJobResponse> getJob(GetJobRequest getJobRequest);
  • Input: getJobRequest — Contains the parameters needed to retrieve a job.
  • Output: Returns a Mono<GetJobResponse>, which includes the job details or status.

3. Delete Job

This API deletes a job based on the provided request.

public Mono<Void> deleteJob(DeleteJobRequest deleteJobRequest);
  • Input: deleteJobRequest — Contains the necessary parameters to delete a job.
  • Output: Returns a Mono<Void> to signify the deletion operation has been completed.

Models

The rest of the classes in this SDK are models that are constructed using the getter/setter pattern,


Tests

The testing section includes:

  • Unit Tests: Focus on validating the functionality of individual components.
  • Integration Tests: Ensure the system's interaction with external dependencies (e.g., the Dapr sidecar) works as expected.

Issue Reference

We strive to ensure that all PRs are opened based on a relevant issue, where the problem or feature has been discussed prior to implementation.

This PR addresses and closes the following issue: #1101, #1300


Checklist

Please make sure you've completed the relevant tasks for this PR:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@siri-varma siri-varma requested review from a team as code owners March 12, 2025 05:15
@siri-varma siri-varma force-pushed the users/svegiraju/cron-2 branch from dabf1dd to 3bfeca2 Compare March 12, 2025 05:20
Copy link
Contributor

@salaboy salaboy left a comment

Choose a reason for hiding this comment

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

@siri-varma can you take a look at my comments? @artur-ciocanu I am curious about your feedback here too.

@artur-ciocanu
Copy link
Contributor

@siri-varma thanks for all your work! I have reviewed the PR and I wanted to share a few general observations/recommendations:

  • As far as I saw different maintainers decided to approach modularity differently. In Dotnet SDK we have separate DLLs for different functionality, in Python everything is in a single client, but the APIs are suffixed with alpha1. In Java SDK we have a special interface called DaprPreviewClient that should host all the alpha APIs. I would recommend having a single client and keep everything in sdk module and use DaprPreviewClient for alpha APIs like Jobs API and Conversation API
  • Similar to above, let's push everything to sdk module, so we avoid creating additional Maven modules
  • Since Jobs API is still alpha, I think it is too early to commit to creating our own CRON expression builder. As far as I can tell a lot of code in this PR is dedicated to CRON expressions. For now I think it is enough to just use CRON expressions as string value. The only thing that we should make sure is that the CRON expression is valid, although I think if it is not, the Dapr sidecar will complain.
  • In the Jobs API that have been added there is a lot of repetition regarding validating model fields like something is not null etc, I would suggest extracting all those validations to separate methods like:
    • validateScheduleJobRequest(...) - to validate ScheduleJobRequest
    • validateGetJobRequest(...) - to validate GetJobRequest
    • validateDeleteJobRequest(...) - to validate DeleteJobRequest
      This way we will have clean separation between data validation and GRPC method invocation.
  • You might want to look into other APIs to make sure that you use the right interceptors to ensure tracing data is properly propagated. Although we could address this later, as well.

CC: @salaboy @cicoyle

Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

@siri-varma please check my comment and let me know your thoughts.

@siri-varma
Copy link
Contributor Author

siri-varma commented Mar 17, 2025

Had a discussion with @artur-ciocanu regarding the comments and this is what we were thinking

You might want to look into other APIs to make sure that you use the right interceptors to ensure tracing data is properly propagated. Although we could address this later, as well.
The only thing that we should make sure is that the CRON expression is valid

Will address the above in a different PR

Will also move CronExpressionBuilder to a different PR for two reasons

  • Because of the amount of logic present in the builder
  • We also want to discuss how the SDK should support such a builder while considering both plain Java and Spring implementations.

cc: @salaboy / @cicoyle

@siri-varma
Copy link
Contributor Author

@artur-ciocanu Addressed all the comments

@siri-varma
Copy link
Contributor Author

@siri-varma thanks for all your work! I have reviewed the PR and I wanted to share a few general observations/recommendations:

  • As far as I saw different maintainers decided to approach modularity differently. In Dotnet SDK we have separate DLLs for different functionality, in Python everything is in a single client, but the APIs are suffixed with alpha1. In Java SDK we have a special interface called DaprPreviewClient that should host all the alpha APIs. I would recommend having a single client and keep everything in sdk module and use DaprPreviewClient for alpha APIs like Jobs API and Conversation API

  • Similar to above, let's push everything to sdk module, so we avoid creating additional Maven modules

  • Since Jobs API is still alpha, I think it is too early to commit to creating our own CRON expression builder. As far as I can tell a lot of code in this PR is dedicated to CRON expressions. For now I think it is enough to just use CRON expressions as string value. The only thing that we should make sure is that the CRON expression is valid, although I think if it is not, the Dapr sidecar will complain.

  • In the Jobs API that have been added there is a lot of repetition regarding validating model fields like something is not null etc, I would suggest extracting all those validations to separate methods like:

    • validateScheduleJobRequest(...) - to validate ScheduleJobRequest
    • validateGetJobRequest(...) - to validate GetJobRequest
    • validateDeleteJobRequest(...) - to validate DeleteJobRequest
      This way we will have clean separation between data validation and GRPC method invocation.
  • You might want to look into other APIs to make sure that you use the right interceptors to ensure tracing data is properly propagated. Although we could address this later, as well.

CC: @salaboy @cicoyle

Addressed the comments

@siri-varma
Copy link
Contributor Author

/assign

@salaboy salaboy mentioned this pull request Mar 19, 2025
3 tasks
@siri-varma siri-varma force-pushed the users/svegiraju/cron-2 branch from fc63a33 to 1fddb66 Compare March 26, 2025 13:13
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

@siri-varma I have left some comments. Could you please take a look and let me know your thoughts.

Thank you.

@cicoyle cicoyle added this to the v1.15 milestone Mar 27, 2025
@artur-ciocanu
Copy link
Contributor

@siri-varma could you please take a look at the tests, it seems that the scheduler container is not working as expected. Also I am wondering if we could avoid using latest tag in containers images, so we have more control when Dapr container group is upgraded.

@artur-ciocanu
Copy link
Contributor

@siri-varma I see the following errors for IT tests:

DaprComponentTest.containerConfigurationTest:42 » ContainerLaunch Container startup failed for image daprio/scheduler:latest

And

Container startup failed for image daprio/daprd:1.14.4

Could you please make sure you use the latest Dapr container 1.15.3 and ensure that the same image tag is used for all the other Dapr containers that are started behind the scenes, like scheduler or placement container.

@siri-varma
Copy link
Contributor Author

@artur-ciocanu I am traveling and will be back in couple of days. Will update the pr by Friday

@siri-varma siri-varma force-pushed the users/svegiraju/cron-2 branch from 5afd23d to a709bdb Compare April 6, 2025 06:10
@siri-varma
Copy link
Contributor Author

@artur-ciocanu Addressed all the comments, fixed integration tests

@siri-varma
Copy link
Contributor Author

The 1.14.4 is coming from actors. I will update it in a separate pr. Will create an issue

salaboy
salaboy previously approved these changes Apr 7, 2025
Copy link
Contributor

@salaboy salaboy left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.. @siri-varma did you wanted to make more changes? Is there something still not address from @artur-ciocanu comments?

@siri-varma
Copy link
Contributor Author

siri-varma commented Apr 7, 2025

@salaboy Thanks for the review and nope, I don't have anything else to address

@siri-varma
Copy link
Contributor Author

#1296

Created an issue and a PR to address the dapr runtime version consistency

@artur-ciocanu
Copy link
Contributor

@siri-varma could you please make sure that you do not accidentally replace fully qualified imports with *. Please make sure you configure your IDE do not collapse multiple imports into a single *.

Also I think it would be cleaner to wait for your PR #1299 and then fix the conflicts in this PR and then merge.

Signed-off-by: sirivarma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
@siri-varma siri-varma force-pushed the users/svegiraju/cron-2 branch from 58fb53a to ea74912 Compare April 12, 2025 04:33
Signed-off-by: siri-varma <[email protected]>
@siri-varma siri-varma force-pushed the users/svegiraju/cron-2 branch from ea74912 to 17429d6 Compare April 12, 2025 04:36
@siri-varma
Copy link
Contributor Author

@cicoyle I have added the Jobs sdk example with markdown in this commit 17429d6

Please take a look. Thank you

Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
@siri-varma
Copy link
Contributor Author

@artur-ciocanu Please look at this commit to 68568c5. I have reduced the spaces from 4 to 2

Signed-off-by: Siri Varma Vegiraju <[email protected]>
artur-ciocanu
artur-ciocanu previously approved these changes Apr 12, 2025
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

Great work!

@artur-ciocanu
Copy link
Contributor

@cicoyle this PR looks good to be merged, please take a look and let's get it in. Thank you.

@siri-varma
Copy link
Contributor Author

@cicoyle let me know if I need to do anything else for merging the pr

Signed-off-by: Siri Varma Vegiraju <[email protected]>
Signed-off-by: Siri Varma Vegiraju <[email protected]>
Signed-off-by: Siri Varma Vegiraju <[email protected]>
Signed-off-by: Siri Varma Vegiraju <[email protected]>
@cicoyle cicoyle merged commit ed9a3fb into dapr:master Apr 16, 2025
8 of 9 checks passed
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 90.50279% with 17 lines in your changes missing coverage. Please review.

Project coverage is 75.88%. Comparing base (d759c53) to head (d18bad6).
Report is 128 commits behind head on master.

Files with missing lines Patch % Lines
...ain/java/io/dapr/testcontainers/DaprContainer.java 62.50% 5 Missing and 1 partial ⚠️
...io/dapr/testcontainers/DaprSchedulerContainer.java 73.68% 5 Missing ⚠️
...k/src/main/java/io/dapr/client/DaprClientImpl.java 97.18% 1 Missing and 1 partial ⚠️
...ain/java/io/dapr/client/domain/GetJobResponse.java 91.66% 2 Missing ⚠️
...java/io/dapr/client/domain/ScheduleJobRequest.java 91.66% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1255      +/-   ##
============================================
- Coverage     76.91%   75.88%   -1.03%     
- Complexity     1592     1684      +92     
============================================
  Files           145      200      +55     
  Lines          4843     5267     +424     
  Branches        562      574      +12     
============================================
+ Hits           3725     3997     +272     
- Misses          821      949     +128     
- Partials        297      321      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cicoyle
Copy link
Contributor

cicoyle commented Apr 16, 2025

Thank you for getting the Jobs API over the line @siri-varma 🙌🏻

@siri-varma siri-varma deleted the users/svegiraju/cron-2 branch April 16, 2025 16:31
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.

6 participants