-
Notifications
You must be signed in to change notification settings - Fork 8
Manoj/steps Minimal steps implementation #4
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
Conversation
| * Helper method for tests | ||
| * Should be moved to TestUtils | ||
| */ | ||
| public void deleteWorkflowsTestHelper() throws SQLException{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better, replace this with a generic "reset system database" method that drops the entire database and can be used in our tests and exposed to help users write their own tests. We have something similar in TS + Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will develop this as we go along
| public void deleteOperations() throws SQLException{ | ||
|
|
||
| String sql = "delete from dbos.workflow_status"; | ||
| String sql = "delete from dbos.operation_outputs;"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we wouldn't need this if we're dropping the whole database.
|
|
||
| private ServiceWFAndStep self ; | ||
|
|
||
| public void setSelf(ServiceWFAndStep serviceWFAndStep) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you call a step from a workflow , the call needs to go through a proxy. The method can be named anything. It is setSelf because I have called the variable self.
In the other test, there were 2 services. There was setServiceB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is this is a way to set self to a proxy. This way, workflows can invoke its own steps.
| @@ -0,0 +1,10 @@ | |||
| package dev.dbos.transact.step; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it's not idiomatic Java, but is it possible for the test classes to be co-located with their tests as in the other languages? It will be easier to write tests in the future if they aren't spread across many files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maven and gradle build expect things in certain directories. The directory structure is dictated by that and every java developer is used to it. it can be changed but it would be hard for us and for any user who tries to get the code and build it
| @@ -0,0 +1,31 @@ | |||
| package dev.dbos.transact.workflow.internal; | |||
|
|
|||
| public class StepResult { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is the difference between this and StepInfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this is a purely internal thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is none. But in both TS and Python , there is internal calls like OperationResultInternal and a public StepInfo ... which are both the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you are right. We could just keep one
kraftp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, this is great progress!
read and write operation_outputs
list steps
Workflow can invoke steps