-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: workflows support in java proto sdk #1971
feat: workflows support in java proto sdk #1971
Conversation
@@ -303,6 +303,8 @@ final class WorkflowImpl(system: ActorSystem, val services: Map[String, Workflow | |||
Future.successful(toProtoEffect(effect, command.id)) | |||
|
|||
case Step(executeStep) => | |||
val context = |
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.
Context is required to call other components.
Ready for the first round, just the codegen updates. Samples, and docs in the following PRs, because this one is big enough. |
One good thing to try out with the refactoring, is to build a proto sample with current latest SDK, then bump to a local version including these changes and see if things break without deleting the old generated sources (user would have their own implementation in them so not so easy to just delete-regenerate) |
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 so far, one problem with the entity type change.
Except for that I think next step is to add a sample with a workflow? (Edit: saw you plan to follow up PR that, I think in general it is good to do sample together with the actual feature because you may notice problems and better to have them right in the feature PR, rather than wrong broken merged to main, but do as you prefer)
...k-protobuf/src/main/scala/kalix/scalasdk/eventsourcedentity/EventSourcedEntityProvider.scala
Show resolved
Hide resolved
I tested that, and |
f87346d
to
b516d07
Compare
service.componentFullName, | ||
throw new IllegalArgumentException( | ||
"Service [" + service.messageType.fullyQualifiedProtoName + "] refers to entity [" + service.componentFullName + | ||
s"], but no entity configuration is found for that component name. Entities: [${entities.keySet.mkString(", ")}]")) | ||
"Service [" + service.messageType.fullyQualifiedProtoName + "] refers to stateful [" + service.componentFullName + |
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.
refers to stateful ["abc"]
vs refers to component ["abc"]
or refers to stateful component ["abc"]
?
Also, why using both concatenation (+
) and string interpolation (s
) together?
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, I think that mixing was used to avoid very long line.
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.
LGTM
We can merge as such, but we still need to add compilation tests for generated code. Can be added in follow-up as this PR is already massive.
|
||
s"""package $packageName; | ||
| | ||
|${writeImports(imports)} |
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.
It's pity that we didn't refactored the java codegen to use our c
interpolator. Check out the scala codegen for more details.
It's much easier to use the c
interpolator and then we get the imports right out of the box.
We should do some gardening in the SDK to solve this kind of issues.
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.
issue created #1991
* feat: workflows support in java proto sdk * deprecated since * codegen warning about deprecated usage of entity_type * warnings about depreactions from the codegen * runtime bump * codegen with all possible descriptors for workflows * updating scala samples * typo removed * improving generated integration test * using pre released version of the runtime * using pre released version of the runtime * PR comments
References