Add basic workflow #3

zklgame · 2023-10-07T16:06:51Z

No description provided.

zklgame · 2023-10-07T16:08:19Z

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

-            request.startStateConfig(processOptions.getStartStateConfig().get());
-        }
-
+    public String startProcess(final ProcessExecutionStartRequest request) {


Use xdb public request as the input to BasicClient to reduce the SDK side wrapper.

longquanzheng · 2023-10-07T21:01:06Z

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

+        return Strings.isNullOrEmpty(namespace) ? DEFAULT_NAMESPACE : namespace;
+    }
+
+    public String getType(final Class<? extends Process> processClass) {


It maybe better to move it to Process class as a package private method(, so that it doesn't require to pass in any parameter. ), or keep it in ProcessUtil

Passing a process class here is a bit strange

Another idea: maybe it's even better to require passing in a Process instance when creating this ProcessOptions (is it possible in lombok to have a required field?, looks like by default all fields are optional in lombok --- looks like just put @NOT_NULL ?)

In the Process interface:

public interface Process{ default getProcessOptions(){ return ProcessOptions.build(this) } }

And we can make this build method as a helper to provide more parameters, like timeout, etc

In lombok builder, the default value of each field is NULL. Adding @NonNull will make sure the specified field must be assigned with a non-null value at the RUNTIME.

Use ProcessOptionsBuilder builder(final Class<? extends Process> processClass) is a good idea to provide ProcessOptions.builder(this.getClass()).build().

Lombok also provides another way to genetate the ProcessOptionsBuilder by new ProcessOptions.ProcessOptionsBuilder(), I didn't find a way to disable it. So when using the new method to generate the builder, users have to be aware the processClass must be set explicitly.

So when using the new method to generate the builder, users have to be aware the processClass must be set explicitly.
yeah that's true. That's why it would be nice if the field being required is clear.
Though, usually when there is a helper build which is easier to use, users would not use the raw/harder one.

longquanzheng · 2023-10-07T21:09:55Z

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

+
+    private final String id;
+
+    public String getId(final Class<? extends AsyncState> stateClass) {


similar here, it seems better to pass in the state instance as required for builder?

longquanzheng · 2023-10-07T21:13:03Z

src/main/java/io/xdb/core/utils/ProcessUtil.java


 public class ProcessUtil {

+    public static final String DEFAULT_NAMESPACE = "default";


nit: I would feel slightly more natural if it's under ProcessOptions. Since it's a static field, it won't mess up with the lombok, I guess

longquanzheng · 2023-10-07T21:14:52Z

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

+        return new AsyncStateExecuteResponse().stateDecision(toServerModel(request.getProcessType(), stateDecision));
+    }
+
+    private StateDecision toServerModel(final String processType, final io.xdb.core.state.StateDecision stateDecision) {


nit: toApiModel or toInternalModels. the api is between server and SDKs, technically doesn't have to be owned by server

longquanzheng

overall looks great, just some small comments

longquanzheng · 2023-10-07T21:16:22Z

src/test/java/integ/basic/BasicProcess.java

+    public static final Integer INPUT = 11;
+
+    @Override
+    public @NonNull StateSchema getStateSchema() {


I guess @nonnull is not required right? It will look a little cleaner if it's not required

@nonnull will make sure the StateSchema will not be null at runtime. So that we don't need to check if (getStateSchema() == null) in case the user return null for the method. Adding it can reduce the checking logic on the SDK side.

I feel like we don't have to enforce this. Our SDK can be smart enough to consider a null as empty schema.

Got it. Consider a null as empty schema sounds like a more user-friendly approach. I will remove the nonnull annotation and deal with the null cases.

longquanzheng · 2023-10-07T21:17:37Z

src/test/java/integ/basic/BasicProcess.java

+        System.out.println("BasicStartingState.waitUntil: " + input);
+        Assertions.assertEquals(INPUT, input);
+
+        return new CommandRequest().waitingType(CommandWaitingType.EMPTYCOMMAND);


nit: it would be better if it provide a helper:

CommandRequest.empty(); or CommandRequest.empty;

True, when we are ready for other CommandWaitingTypes, we should add helper like StateDecision. I will add it that time.

Yeah no problem. You may want it when start writing samples or more integ tests because it will save time. We can always change it later before final launch. When it’s time to share with more people we will have another review to refine the APIs

longquanzheng · 2023-10-07T21:18:44Z

src/test/java/integ/basic/BasicProcess.java

+        Assertions.assertEquals(INPUT, input);
+
+        return StateDecision.multipleNextStates(
+            StateMovement.builder().stateId(STATE_ID_NEXT_1).stateInput(input + 1).build(),


similar here, it would be nicer to have a helper:

Statemovement.create(stateId [class/string], input);

zklgame added 2 commits October 7, 2023 20:10

simplify

76b347b

add integ test

3647b42

zklgame commented Oct 7, 2023

View reviewed changes

refine

de12b57

zklgame marked this pull request as ready for review October 7, 2023 16:35

zklgame requested a review from longquanzheng October 7, 2023 16:35

longquanzheng reviewed Oct 7, 2023

View reviewed changes

longquanzheng approved these changes Oct 7, 2023

View reviewed changes

zklgame added 3 commits October 8, 2023 09:59

refine

391e88f

remove nonnull and handle null cases

c705388

fix the getId/getType

bd56d73

zklgame merged commit fa421f3 into main Oct 8, 2023
1 check passed

zklgame deleted the add_basic_workflow branch October 8, 2023 05:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic workflow #3

Add basic workflow #3

zklgame commented Oct 7, 2023

zklgame Oct 7, 2023

longquanzheng Oct 7, 2023 •

edited

Loading

longquanzheng Oct 7, 2023 •

edited

Loading

zklgame Oct 8, 2023

longquanzheng Oct 8, 2023

longquanzheng Oct 7, 2023

longquanzheng Oct 7, 2023 •

edited

Loading

longquanzheng Oct 7, 2023

longquanzheng left a comment •

edited

Loading

longquanzheng Oct 7, 2023

zklgame Oct 8, 2023 •

edited

Loading

longquanzheng Oct 8, 2023

zklgame Oct 8, 2023

longquanzheng Oct 7, 2023

zklgame Oct 8, 2023

longquanzheng Oct 8, 2023 •

edited

Loading

longquanzheng Oct 7, 2023


		private final String id;

		public String getId(final Class<? extends AsyncState> stateClass) {


		public class ProcessUtil {

		public static final String DEFAULT_NAMESPACE = "default";

Add basic workflow #3

Add basic workflow #3

Conversation

zklgame commented Oct 7, 2023

Choose a reason for hiding this comment

longquanzheng Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

longquanzheng Oct 7, 2023 • 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

longquanzheng Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longquanzheng left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zklgame Oct 8, 2023 • 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

longquanzheng Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longquanzheng Oct 7, 2023 •

edited

Loading

longquanzheng Oct 7, 2023 •

edited

Loading

longquanzheng Oct 7, 2023 •

edited

Loading

longquanzheng left a comment •

edited

Loading

zklgame Oct 8, 2023 •

edited

Loading

longquanzheng Oct 8, 2023 •

edited

Loading