Setup: add skeleton #2

zklgame · 2023-09-30T08:08:32Z

No description provided.

longquanzheng

Overall looks great. God job!

longquanzheng · 2023-09-30T16:24:00Z

README.md

+## 1.0
+
+- [ ] StartProcessExecution API
+  - [] Basic


typo: [ ] with a space otherwise it will not look right

longquanzheng · 2023-09-30T16:24:55Z

src/main/java/io/xdb/core/client/BasicClient.java

+/**
+ * {@link BasicClient} serves as a foundational client without a process {@link io.xdb.core.registry}.
+ * It represents the internal implementation of the {@link Client}.
+ * However, it can also be utilized directly if there is a compelling reason, allowing you to invoke APIs on the xdb server with minimal type validation checks, such as process type, queue names, and so forth.


with minimal type validation checks

I think it's no type check actually.

True. I just copied this descrption from the golang-sdk.

Ah I will fix that

longquanzheng · 2023-09-30T16:27:18Z

src/main/java/io/xdb/core/client/Client.java

+    }
+
+    public String startProcess(
+        final String processType,


It would be better to use Process processDefinition to make it more "strongly typing experience".

Sorry 😢 I forgot this. In Java it’s even better to just use the class type like this https://github.com/indeedeng/iwf-java-sdk/blob/d2ae8439d4b7ed996c20303be57f248eb00fe4ef/src/main/java/io/iworkflow/core/Client.java#L63 override process/workflow type is really rare and they can probably use basic client instead

I think users will be confused when they define type in the options but pass the class in the startProcess method, and find the customized type is not working.

So let's keep using the definition here to avoid such confusion.

We can provide both the string processType and the Class like iwf-java-sdk.

Or even just not allow customizing processType for now. It's really not useful so far. In our new python SDK, I just don't expose the workflowType customization: https://github.com/indeedeng/iwf-python-sdk/blob/main/iwf/workflow.py#L8 And it's not late to add it later when needed.

On the other side, using Class is much more friendly than using the workflow instance, it save the user code for dependency injection. As a result, I deleted this one in Java SDK recently: indeedeng/iwf-java-sdk#195 (still keep the string WorkflowType)

longquanzheng · 2023-09-30T16:33:36Z

src/main/java/io/xdb/core/exception/XDBHttpException.java

+        final FeignException.FeignClientException exception
+    ) {
+        if (exception.status() >= 400 && exception.status() < 500) {
+            return new ClientSideException(objectEncoder, exception);


for 400<statusCode<500, can you add some more friendly exceptions (use final exceptions):
404: ProcessNotFoundException
409: ProcessAlreadyStartedException

This is also something that I want to improve in iWF: indeedeng/iwf-java-sdk#203

and we have done the same in Python SDK later: https://github.com/indeedeng/iwf-python-sdk/blob/main/iwf/errors.py

longquanzheng · 2023-09-30T16:37:29Z

src/main/java/io/xdb/core/process/BasicClientProcessOptions.java

+public class BasicClientProcessOptions {
+
+    private final Optional<ProcessOptions> processOptionsOptional;
+    private final AsyncStateConfig startStateConfig;


startStateConfig should also be optional

longquanzheng · 2023-09-30T16:37:54Z

src/main/java/io/xdb/core/process/BasicClientProcessOptions.java

+    private final AsyncStateConfig startStateConfig;
+
+    public BasicClientProcessOptions(final ProcessOptions processOptions, final AsyncStateConfig startStateConfig) {
+        if (processOptions == null) {


Nit: I think it can be just Optional.ofNullable(processOptions)

Good learning!

longquanzheng · 2023-09-30T16:44:01Z

src/main/java/io/xdb/core/process/Process.java

+ * It represents a fundamental concept at the top level in XDB.
+ */
+public interface Process {
+    default String getType() {


I am thinking to do this for Golang SDK recently --

What do you think that we move the ProcessOptions here so that ProcessOption will include this Type, and others like timeouts, IdReusePolicy?

Because usually those timeout/IdReusePolicy can be determined when implementing the process definition, rather than being specified when starting a process.

Another reasons is that I am hoping to have a more extensible struct to to represent those "optional configuration" for a process definition.

That's a good idea. I am going to implment in this way here.

Just tried in Java and found that if moving the type out of Process, we cannot provide the default value as the class.getSimpleName() because now the type is not included in the class def.

We can do it in the Golang way — use an empty value to represent the default value. When resolving the process type , we can use class name

Makes sense.

longquanzheng · 2023-09-30T16:47:51Z

src/main/java/io/xdb/core/state/AsyncState.java

+     *
+     * @return the state id
+     */
+    default String getId() {


then similarly, we can use AsyncStateOptions here to merge it with our future getAsyncStateOptions

longquanzheng · 2023-09-30T16:49:18Z

src/main/java/io/xdb/core/worker/WorkerService.java

+
+public class WorkerService {
+
+    private static final String API_PATH_ASYNC_STATE_WAIT_UNTIL = "/api/v1/xdb/worker/async-state/wait-until";


they need to be public so that user code can use

longquanzheng · 2023-09-30T16:52:01Z

src/main/java/io/xdb/core/client/ServerApiRetryConfig.java

+
+    public static ServerApiRetryConfig getDefault() {
+        return ServerApiRetryConfig
+            .builder()


Nice, looks like lombok is better/friendly than immutable 👍 I had to manually add those builder helpers myself when using immutable

longquanzheng

just some final minor comments

zklgame added 3 commits September 29, 2023 23:37

setup

78cc8ae

Setup

a4496c1

add tests

9825cd5

zklgame force-pushed the setup branch from bcdcfc9 to 9825cd5 Compare September 30, 2023 08:46

zklgame changed the title ~~Setup~~ Setup: add skeleton Sep 30, 2023

zklgame requested a review from longquanzheng September 30, 2023 08:48

longquanzheng reviewed Sep 30, 2023

View reviewed changes

zklgame added 2 commits October 2, 2023 09:49

refine

f352a89

refine

183562d

zklgame requested a review from longquanzheng October 2, 2023 00:52

refine

cabea27

longquanzheng approved these changes Oct 2, 2023

View reviewed changes

add method

cac9406

zklgame merged commit 40df895 into main Oct 3, 2023
1 check passed

zklgame deleted the setup branch October 3, 2023 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup: add skeleton #2

Setup: add skeleton #2

zklgame commented Sep 30, 2023

longquanzheng left a comment

longquanzheng Sep 30, 2023

longquanzheng Sep 30, 2023

zklgame Oct 2, 2023

longquanzheng Oct 2, 2023

longquanzheng Sep 30, 2023

longquanzheng Oct 2, 2023 •

edited

zklgame Oct 2, 2023

longquanzheng Oct 2, 2023 •

edited

longquanzheng Sep 30, 2023

longquanzheng Sep 30, 2023

longquanzheng Sep 30, 2023

zklgame Oct 2, 2023

longquanzheng Sep 30, 2023

zklgame Oct 2, 2023

zklgame Oct 2, 2023

longquanzheng Oct 2, 2023

zklgame Oct 2, 2023

longquanzheng Sep 30, 2023

longquanzheng Sep 30, 2023

longquanzheng Sep 30, 2023

longquanzheng left a comment


		public class WorkerService {

		private static final String API_PATH_ASYNC_STATE_WAIT_UNTIL = "/api/v1/xdb/worker/async-state/wait-until";

Setup: add skeleton #2

Setup: add skeleton #2

Conversation

zklgame commented Sep 30, 2023

longquanzheng 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

longquanzheng Oct 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longquanzheng Oct 2, 2023 • edited

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

longquanzheng left a comment

Choose a reason for hiding this comment

longquanzheng Oct 2, 2023 •

edited

longquanzheng Oct 2, 2023 •

edited