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

[WORK IN PROGRESS] Add DSL Sample #47

Closed
wants to merge 3 commits into from

Conversation

darewreck54
Copy link

Add a dsl example. This is to port the Go Example.

There were issue with the previous #45, so created a new one.

public class Statement {
public ActivityInvocation activity;
public Sequence sequence;
public Parrallel parrallel;
Copy link
Author

Choose a reason for hiding this comment

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

@mfateev You made a comment about using inheritance. Can you give me an example of what you mean? I wasn't following what can inherit from what?

Copy link
Member

Choose a reason for hiding this comment

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

interface Statement {}
class Sequence implements Statement {}
class ActivityInvocation implements Statement {}
class Parallel implements Statement {}

Copy link
Author

Choose a reason for hiding this comment

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

I'm not seeing what the interface could be. Here is wha we have?

class Statement {
   public ActivityInvocation activity;
   public Sequence sequence;
   public Parallel parallel;
}

class Sequence {
  public Statement elements[];
}

class Parallel  {
  public Statement branches[];
}

class ActivityInvocation {
  public String name;
  public String method;
  public String[] arguments;
  public String result;
}

The only thing similar is sequence and parallel, but you would have to change the name to be consistent elements vs. branches.

Copy link
Author

Choose a reason for hiding this comment

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

@mfateev ping

@darewreck54
Copy link
Author

@mfateev I addressed the comments that I understood and added questions in the code. If you have time, please take a look and let me know how I can proceed.

Thanks!

Copy link
Member

@mfateev mfateev left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution!

import io.temporal.activity.ActivityInterface;
import io.temporal.activity.ActivityMethod;

public class SampleActivities {
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity let's use a single activity interface with multiple methods instead of many interfaces.

Copy link
Author

Choose a reason for hiding this comment

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

I think leaving as it shows the user how you would use the name_interface in the case a user wants to go with that approach (ex. I wanted to know and it wasn't clear to me). But if you want me to change it, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would change it. We can create another example that demonstrates multiple Activity interfaces.

.setTaskQueue("dsl")
.build());

String results = stub.execute(this.method, String.class, new Object[] {args});
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to change activities to accept the args as the current implementation doesn't use them at all.
I'm not sure why new Object[] {args} is needed instead of just args.

Copy link
Author

Choose a reason for hiding this comment

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

If you don't do this, you get a warning in java. The method takes a list of args which need to be expanded. This was the workaround I found on stack-overflow to get around it.

It would be nice to change activities to accept the args as the current implementation doesn't use them at all.

Doesn't passing in the args like that use it in the activities? When I look at the stack trace, it looks like it's using it as inputs in the payload. Were you referring to something else?

image

public class Statement {
public ActivityInvocation activity;
public Sequence sequence;
public Parrallel parrallel;
Copy link
Member

Choose a reason for hiding this comment

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

interface Statement {}
class Sequence implements Statement {}
class ActivityInvocation implements Statement {}
class Parallel implements Statement {}

src/main/java/io/temporal/samples/dsl/models/Workflow.java Outdated Show resolved Hide resolved
src/main/resources/dsl/workflow1.yaml Outdated Show resolved Hide resolved
import io.temporal.activity.ActivityInterface;
import io.temporal.activity.ActivityMethod;

public class SampleActivities {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would change it. We can create another example that demonstrates multiple Activity interfaces.

@@ -36,7 +36,9 @@ public Void execute(Map<String, String> bindings) {
.setTaskQueue("dsl")
.build());

String results = stub.execute(this.method, String.class, new Object[] {args});
String methodNameDescriptor =
this.name + this.method.substring(0, 1).toUpperCase() + this.method.substring(1);
Copy link
Member

Choose a reason for hiding this comment

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

I would use a single "activityType" string. Activities can be implemented in different languages like Go which have completely different approach to activity naming.


public void execute(Map<String, String> bindings) {
// In the parallel block, we want to execute all of them in parallel and wait for all of them.
// if one activity fails then we want to cancel all the rest of them as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if one activity fails then we want to cancel all the rest of them as well.
// if one Activity Execution fails then we want to cancel all the rest of them as well.

@tsurdilo
Copy link
Contributor

tsurdilo commented Jul 7, 2021

This sample is fairly outdated and we are working on a new DSL example. I will close this for now and if there is any more inputs/additions you would like to add here please feel free to reopen it.

@tsurdilo tsurdilo closed this Jul 7, 2021
@darewreck54
Copy link
Author

@tsurdilo Sorry, i've been busy so I haven't had time to update this. Would you like me to push the changes or do you think its not needed since it sounds like you have a new DSL example?

Also mind if i see what your new DSL example looks like?

@tsurdilo
Copy link
Contributor

tsurdilo commented Jul 7, 2021

@darewreck54 up to you I can reopen this again, np :)
I don't have the PR ready yet, it's using DynamicWorkflow and DynamicInterface. Will ping you when its ready for review.

@tsurdilo tsurdilo reopened this Jul 7, 2021
@tsurdilo tsurdilo changed the title Feat(DSL Sample): Add DSL Sample [WORK IN PROGRESS] Add DSL Sample Jul 7, 2021
@tsurdilo
Copy link
Contributor

tsurdilo commented Sep 9, 2021

@darewreck54 created #151
will close this pr. feel free to reopen it if you like. thanks!

@tsurdilo tsurdilo closed this Sep 9, 2021
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.

4 participants