WIP: Add Jobs API #1110

charan2628 · 2024-08-21T18:40:34Z

Description

Add Jobs API

Issue reference

Please reference the issue this PR will close: #1101

Checklist

Code compiles correctly
Created/updated tests
Extended the documentation

charan2628 · 2024-08-21T18:41:33Z

@cicoyle I have added scheduleJob API, can you please review once If my approach is right

artursouza

This is a great start. I am thankful for this contribution. Some feedback for it to be aligned to the current style of the interface. Thanks.

artursouza · 2024-08-21T19:12:42Z

sdk/src/main/java/io/dapr/client/DaprClient.java

+   * @param job job to be scheduled
+   * @return a Mono plan of type Void.
+   */
+  <T extends Message> Mono<Void> scheduleJobAlpha1(Job<T> job);


It does not need to be a protobuf message. Although the implementation can be smart enough to handle Message as a special case and not use the serializer.

This interface is agnostic of gRPC protocol.

@artursouza I can't convert generic type T to Any data type. It is expecting generic type to extend Message

@charan2628 you are defining the method signature, what @artursouza is suggesting is not to use Message as the public return type.

@artursouza and @salaboy got it I am using DaprObjectSerializer

artursouza · 2024-08-21T19:13:25Z

sdk/src/main/java/io/dapr/client/domain/Job.java

+ *
+ * @param <T> The class type of Job data.
+ */
+public final class Job<T extends Message> {


Same. Make it gRPC agnostic.

@charan2628 there is no need to make it parameterizable (<T extend Message>) here.

artursouza · 2024-08-21T19:14:01Z

sdk/src/main/java/io/dapr/client/domain/Job.java

+	 * 
+	 * @param name name of the job to create
+	 */
+	public Job(String name, T data) {


The constructor should include all required fields.

@charan2628 you are still missing this.

@charan2628 ideally you should put all the data in constructor and have just "getters" without setters.

@artur-ciocanu as per the spec only name and data are required, remaining are optional that's why I included only these two in constructor, can I two constructors one with mandatory fields and another with all fields?

charan2628 · 2024-08-27T18:21:06Z

sdk/src/main/java/io/dapr/client/DaprClientImpl.java

+
+		  DaprProtos.Job.Builder jobBuilder = DaprProtos.Job.newBuilder()
+				  .setName(name)
+				  .setData(Any.pack(data));


@artursouza here I have to convert data which is generic to Any data type, it is allowing only generics that extend Message

@charan2628 this is only because you defined that in the DaprClient interface. Your Job class doesn't need to extend Message. Check the other domain objects such as: https://github.com/dapr/java-sdk/blob/b167c08b71c2b65cc1685ef167930ed028146dbb/sdk/src/main/java/io/dapr/client/domain/GetStateRequest.java , this is what @artursouza means by make it GRPC agnostic. You receive a GRPC message inside and translate that to a class that doesn't have any dependency to GRPC messages.

salaboy · 2024-08-28T08:35:07Z

@charan2628 I've added more feedback here.. let me know if that makes sense.

charan2628 · 2024-08-29T17:57:43Z

sdk/src/main/java/io/dapr/client/DaprClientImpl.java

+  		  jobBuilder.setData(Any.pack((Message)job.getData()));
+  	  } else {
+  		  jobBuilder.setData(Any.newBuilder().setValue(ByteString.copyFrom(this.objectSerializer.serialize(data))));
+  	  }


@artursouza @salaboy is this implementation fine?

@artursouza the data of the Job can be something different than Message?

@charan2628 @salaboy and @artursouza I see that Job data is modeled as google.protobuf.Any. In other places where we use google.protobuf.Any like TransactionalActorStateOperation, we check that data is either String or byte[]. I think we should stick to this approach.

We don't want to expose com.google.protobuf.Message to DaprClient users, otherwise they will have too many options to create a Job, while having data as either String or byte[] covers most of the use cases.

@salaboy @artur-ciocanu is this implementation fine?

salaboy · 2024-09-05T10:04:04Z

sdk/src/main/java/io/dapr/client/DaprClient.java

+   * @param job job to be scheduled
+   * @return a Mono plan of type Void.
+   */
+  <T> Mono<Void> scheduleJobAlpha1(Job<T> job);


This looks good based on this docs: https://docs.dapr.io/reference/api/jobs_api/

cicoyle · 2024-09-24T19:20:36Z

Gentle ping @charan2628 - did you address the above? Do you need any assistance?

artur-ciocanu

@charan2628 awesome job! Thanks a lot for your PR. I have added a few comments, whenever you have a chance could you please take look.

Thank you! 🙇

artur-ciocanu · 2024-10-02T14:35:05Z

sdk/src/main/java/io/dapr/client/DaprClient.java

+   * @param <T> The type of the data for the job.
+   * @param job job to be scheduled
+   * @return a Mono plan of type Void.
+   */


@charan2628 since this is still early days, we should move it to DaprPreviewClient where we host all the "experimental" APIs.

Thanks for catching this @artur-ciocanu

artur-ciocanu · 2024-10-02T14:36:58Z

sdk/src/main/java/io/dapr/client/domain/Job.java

@@ -0,0 +1,74 @@
+package io.dapr.client.domain;
+


This import is not used, so you should probably remove it

artur-ciocanu · 2024-10-02T14:38:30Z

sdk/src/main/java/io/dapr/client/domain/Job.java

+	 * 
+	 * @param name name of the job to create
+	 */
+	public Job(String name, T data) {


@charan2628 ideally you should put all the data in constructor and have just "getters" without setters.

artur-ciocanu · 2024-10-02T14:49:41Z

sdk/src/main/java/io/dapr/client/DaprClientImpl.java

+  		  jobBuilder.setData(Any.pack((Message)job.getData()));
+  	  } else {
+  		  jobBuilder.setData(Any.newBuilder().setValue(ByteString.copyFrom(this.objectSerializer.serialize(data))));
+  	  }


@charan2628 @salaboy and @artursouza I see that Job data is modeled as google.protobuf.Any. In other places where we use google.protobuf.Any like TransactionalActorStateOperation, we check that data is either String or byte[]. I think we should stick to this approach.

We don't want to expose com.google.protobuf.Message to DaprClient users, otherwise they will have too many options to create a Job, while having data as either String or byte[] covers most of the use cases.

charan2628 · 2024-10-11T06:18:03Z

Gentle ping @charan2628 - did you address the above? Do you need any assistance?
I doing the changes, will push this week

charan2628 · 2024-10-17T18:57:53Z

sdk/src/main/java/io/dapr/client/DaprClientImpl.java

+		  return DaprException.wrapMono(ex);
+	  }
+  }
+


@salaboy @artur-ciocanu this getJobAplha1 implementation is fine? I taking in the type of the data.

charan2628 · 2024-10-17T19:00:13Z

@salaboy @artur-ciocanu I have done the changes mentioned for ScheduleJob API and implemented Delete and GetJob APIs, please review it.

charan2628 requested review from a team as code owners August 21, 2024 18:40

artursouza requested changes Aug 21, 2024

View reviewed changes

cicoyle added this to the v1.13 milestone Aug 26, 2024

charan2628 commented Aug 27, 2024

View reviewed changes

Schedule Job API

5628ef5

charan2628 force-pushed the feature/jobs-api branch from b167c08 to 5628ef5 Compare August 29, 2024 17:56

charan2628 commented Aug 29, 2024

View reviewed changes

salaboy reviewed Sep 5, 2024

View reviewed changes

artur-ciocanu suggested changes Oct 2, 2024

View reviewed changes

Merge branch 'master' into feature/jobs-api

c4541e0

artursouza and others added 3 commits October 16, 2024 07:21

Merge branch 'master' into feature/jobs-api

d8fb6aa

Merge branch 'dapr:master' into feature/jobs-api

591e58d

DeleteJobRequest and GetJobRequest API's

4c208b4

charan2628 commented Oct 17, 2024

View reviewed changes

artursouza modified the milestones: v1.13, v1.14 Oct 22, 2024

Merge branch 'master' into feature/jobs-api

a966fbd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add Jobs API #1110

WIP: Add Jobs API #1110

charan2628 commented Aug 21, 2024

charan2628 commented Aug 21, 2024

artursouza left a comment

artursouza Aug 21, 2024

charan2628 Aug 27, 2024

salaboy Aug 28, 2024

charan2628 Aug 29, 2024

artursouza Aug 21, 2024

salaboy Aug 28, 2024

artursouza Aug 21, 2024

salaboy Sep 5, 2024

artur-ciocanu Oct 2, 2024

charan2628 Oct 17, 2024

charan2628 Aug 27, 2024

salaboy Aug 28, 2024

salaboy commented Aug 28, 2024

charan2628 Aug 29, 2024 •

edited

Loading

salaboy Sep 5, 2024

artur-ciocanu Oct 2, 2024

charan2628 Oct 17, 2024

salaboy Sep 5, 2024

cicoyle commented Sep 24, 2024

artur-ciocanu left a comment

artur-ciocanu Oct 2, 2024

artursouza Oct 8, 2024

artur-ciocanu Oct 2, 2024

artur-ciocanu Oct 2, 2024

artur-ciocanu Oct 2, 2024

charan2628 commented Oct 11, 2024

charan2628 Oct 17, 2024

charan2628 commented Oct 17, 2024

WIP: Add Jobs API #1110

Are you sure you want to change the base?

WIP: Add Jobs API #1110

Conversation

charan2628 commented Aug 21, 2024

Description

Issue reference

Checklist

charan2628 commented Aug 21, 2024

artursouza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salaboy commented Aug 28, 2024

charan2628 Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cicoyle commented Sep 24, 2024

artur-ciocanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charan2628 commented Oct 11, 2024

Choose a reason for hiding this comment

charan2628 commented Oct 17, 2024

charan2628 Aug 29, 2024 •

edited

Loading