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

Java Workflow client naming inconsistent with other Workflow SDKs. #1012

Open
msfussell opened this issue Feb 17, 2024 · 7 comments
Open

Java Workflow client naming inconsistent with other Workflow SDKs. #1012

msfussell opened this issue Feb 17, 2024 · 7 comments
Labels
good first issue Good for newcomers kind/bug Something isn't working P0
Milestone

Comments

@msfussell
Copy link
Member

msfussell commented Feb 17, 2024

The Java SDK Workflow naming is inconsistent with the other SDKs.

For example Currently the Workflow SDK has schedule for Java
String instanceId = client.scheduleNewWorkflow(DemoWorkflow.class, "input data");

when the other SDKs have start. For example .NET
StartWorkflowResponse startResponse = await daprClient.StartWorkflowAsync(orderId, workflowComponent, workflowName, input, workflowOptions);

and Python

start_resp = d.start_workflow(instance_id=instanceId, workflow_component=workflowComponent,
                        workflow_name=workflowName, input=inputData, workflow_options=workflowOptions)

Hence
scheduleNewWorkflow should be renamed to startWorkflow

As another example Java SDK uses 'instance' everywhere and this is inconsistent with using 'workflow' in other SDKs.

 // Get status information on the workflow
      WorkflowInstanceStatus workflowMetadata = client.getInstanceState(instanceId, true);

as opposed to .NET

// Get information on the workflow. This response contains information such as the status of the workflow, when it started, and more!
GetWorkflowResponse getResponse = await daprClient.GetWorkflowAsync(orderId, workflowComponent, eventName);

and Python

# Get info on the workflow
getResponse = d.get_workflow(instance_id=instanceId, workflow_component=workflowComponent)

The docs in both the Java SDK and the Doc repo here https://docs.dapr.io/developing-applications/building-blocks/workflow/howto-manage-workflow/ will also need to be updated with this change

@msfussell msfussell added the kind/bug Something isn't working label Feb 17, 2024
@msfussell msfussell changed the title Workflow Client. Schedule should be called Start in SDK Workflow Client. scheduleNewWorkflow should be renamed to startWorkflow Feb 17, 2024
@msfussell msfussell changed the title Workflow Client. scheduleNewWorkflow should be renamed to startWorkflow Java Workflow client naming inconsistent with other Workflow SDKs. Feb 17, 2024
@cgillum
Copy link

cgillum commented Feb 17, 2024

@msfussell I think you're actually comparing two separate things: the Dapr Workflow SDK (in the Java example) and the Dapr SDK APIs for workflow management (your .NET and Python examples). I understand where this confusion is coming from and I'll try to explain.

For each language, we effectively have two SDKs for managing workflows.

  1. The Dapr client, which defines workflow management APIs
  2. The Dapr Workflow client, which defines both management and authoring APIs.

The reason we have this duplication is because at the start of this project, we were aiming to support multiple workflow engines. That's why, for example, the workflow management APIs on the Dapr Client take a "component name" parameter. These APIs had to be generic for all possible workflow engines. As it turns out, we decided to only support one workflow engine, the "dapr" workflow engine.

The reason we also have Dapr Workflow APIs is because we wanted a way to expose the specific features of Dapr Workflow to developers which may not be applicable for other, external workflow engines. For example, the Dapr Workflow engine has the capability to schedule workflows to start running in the future. This is why, for example, the Dapr Workflow API is called "ScheduleNewWorkflowXXX" whereas the Dapr client API for interfacing with external workflow engines is simply "StartWorkflow".

Going back to the point about consistency, I did a quick audit and it looks like we are being consistent in terms of naming for the respective SDK type across all languages that support workflow.

Language Dapr client Dapr Workflow client
.NET StartWorkflowAsync ScheduleNewWorkflowAsync
Java n/a scheduleNewWorkflow
Python start_workflow schedule_new_workflow
JavaScript start scheduleNewWorkflow
Go StartWorkflowBeta1 ScheduleNewWorkflow

What I want to do is actually deprecate or at least deemphasize the "Dapr client" workflow APIs because they're less useful than the Dapr Workflow variants. However, I recognize that we'd need to go through the formal proposal process for this and make a decision as a larger group. In the meantime, I've been trying to ask contributors to use the Dapr Workflow APIs in the samples, but it looks like that work hasn't been done yet, which is why you discovered these inconsistencies.

@msfussell
Copy link
Member Author

msfussell commented Feb 17, 2024

@cgillum - Thanks for the clarification. Then should the word "instance" be in the public API for Java when it is not in the other client Workflow APIs? That seems inconsistent.

Also this implies the workflow API doc here is needs changing https://v1-13.docs.dapr.io/reference/api/workflow_api/
and the management samples in docs (like the SDK samples) will need changing https://v1-13.docs.dapr.io/developing-applications/building-blocks/workflow/howto-manage-workflow/

And whilst we are changing the samples, can we consider having the same name for the workflow, since currently every SDK sample is different.

I see this as a "must do" issue for the v1.14 timeframe.

@cgillum cgillum added the P0 label Oct 15, 2024
@cicoyle cicoyle added this to the v1.13 milestone Oct 15, 2024
@cicoyle cicoyle added the good first issue Good for newcomers label Oct 15, 2024
@salaboy
Copy link
Contributor

salaboy commented Oct 17, 2024

@cgillum @msfussell I am happy to take that issue, but I need to admit that I am confused and I am not sure about if a decision has been made about the naming yet. As a workflow user I am used to have access to the workflow instance, so I didn't saw anything wrong there when using the API, but I agree that looking at your examples it is inconsistent. I need to dig deeper to what @cgillum has mentioned, I do understand there are two APIs to do different things, so we need to make sure that those are also consistent between each other. If you can help me to make a decision on naming I am happy to get this done.

@cgillum
Copy link

cgillum commented Oct 17, 2024

Thanks @salaboy for offering to help with this! Regarding the deprecation of generic DaprClient APIs for workflows, a decision has been made to go ahead and remove them, and some of that work has been completed for 1.15. We need to confirm that this work is being done for all SDKs, but that should resolve the first naming inconsistency.

Regarding "instance", I don't have a strong opinion, nor am I immediately familiar with the full set of inconsistencies. I suppose one specific term we can consider is "workflow ID" vs. "[workflow] instance ID". I think there are potential ambiguities with both but would be supportive of picking one and using it consistently. @msfussell would love to get your opinion here.

@salaboy
Copy link
Contributor

salaboy commented Oct 17, 2024

WorkflowID is usually the ID that makes reference to a workflow definition, we should be able to identify that with an unique identifier, while each instance of that definition should have its own instance ID.

@salaboy
Copy link
Contributor

salaboy commented Dec 4, 2024

@cgillum @msfussell I didn't worked on this because it is not clear what the decision is. I would recommend against using "WorkflowId" to refer to a Workflow Instance ID :) as the WorkflowId is more related to its name or unique identifier across all workflow definitions.

@cgillum
Copy link

cgillum commented Dec 4, 2024

I'm also in favor of "workflow instance ID" for the reasons mentioned. It's also more consistent with related Durable Task terminology.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/bug Something isn't working P0
Projects
None yet
Development

No branches or pull requests

5 participants