-
Notifications
You must be signed in to change notification settings - Fork 8
Manoj/async #2
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
Manoj/async #2
Conversation
| throw new NonExistentWorkflowException(workflowId) ; | ||
| } | ||
|
|
||
| public List<WorkflowStatus> getWorkflows(GetWorkflowsInput input) 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.
Nit: would be better to rename this listWorkflows and ListWorkflowsInput
| } | ||
|
|
||
| @Override | ||
| public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { |
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.
Why is the proxy not used in this function?
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.
That object is the proxy object on which the method was called.
In most cases it can be ignored. It can be useful in some special cases like needing to pass the proxy as a callback or if the the invoke method needs to call another method on the proxy.
So far I have not needed it
| .async() | ||
| .build(); | ||
|
|
||
| SimpleServiceImpl.executionCount =0 ; |
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.
Nit: is there a Java code formatter that we can use to clean up our code format? In Python we use Black, TS uses prettier.
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.
will look for one
| throw new Exception("DBOS Test error") ; | ||
| } | ||
|
|
||
|
|
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.
In this file, looks like SimpleServiceImpl implements SimpleService but not SimpleAsyncService. Will Java report a compiler error if we write SimpleServiceImpl implements SimpleAsyncService because the interface doesn't match with the implementation (due to WorkflowHandle)?
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 change the test case impl class to not implement any interface so that it is clear
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.
made the naming clearer
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.
As we discussed, for now we can merge the sync and async paths, making every workflow invocation in an executor pool. This will hopefully simplify the interface a lot.
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.
fixed
No description provided.