Skip to content

Conversation

shijiesheng
Copy link
Member

@shijiesheng shijiesheng commented May 15, 2025

What changed?

  • added client entities that maps 1:1 with thrift (generated from a generator)
  • added BaseError, which is a catchall similar to TException.
  • added mappers to transform Proto to client entities. (mostly copied from thrift mappers)
  • added IWorkflowServiceV4 which is exactly same as IWorkflowService except for it's using the client entities.
  • added WorkflowServiceGrpc that implements the V4 interface

Plan

  • new V4 interface with new entities < This PR
  • replace all pointers to V4 in one go (Should be import change only)

Why?

Thrift deprecation work

How did you test it?

Unit Test on mappers from ClientObjects to ProtoObjects

Potential risks

Release notes

Documentation Changes

@dkrotx
Copy link
Member

dkrotx commented May 26, 2025

Is this a complete switch to GRPC, so we're throwing away thrift?
I wonder how much of the diff is auto-generated and with one worth checking? the diff looks rather huge.

@shijiesheng
Copy link
Member Author

Is this a complete switch to GRPC, so we're throwing away thrift? I wonder how much of the diff is auto-generated and with one worth checking? the diff looks rather huge.

I finally get the chance to add the unit test for mappers.

To answer your question, I mainly need reviews on 1:1 comparison of thrift vs newly added entities. Others are captured by mappers tests.

build.gradle Outdated
compile group: 'com.google.api.grpc', name: 'proto-google-common-protos', version: '2.10.0'
compile group: 'com.google.protobuf', name: 'protobuf-java-util', version: '3.21.9'
compile group: 'com.google.oauth-client', name: 'google-oauth-client', version: '1.35.0'
compile group: 'org.projectlombok', name: 'lombok', version: '1.18.30'
Copy link
Member

Choose a reason for hiding this comment

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

We should only need the compileOnly dependency. We don't want to include this at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

class DecisionMapper {
static List<Decision> decisionArray(List<com.uber.cadence.entities.Decision> t) {
if (t == null) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

This might cause NPE.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

build.gradle Outdated
}
java {
srcDir 'src/main'
srcDir '.gen'
Copy link
Member

Choose a reason for hiding this comment

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

With idiomatic Java you pretty much never check in generated sources (unlike Go).

But we don't want to run the generator as part of the build step and want to eventually delete the generator entirely. At which point we'd want to put it in src/main/java with all the other sources.

I think it makes the most sense to put it in like src/gen/java/ and then we merge it into src/main/java later.

The .gen directory pattern just seems really strange from a Java perspective.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@shijiesheng shijiesheng merged commit aafd9dd into cadence-workflow:v4.x.x Jun 13, 2025
7 checks passed
shijiesheng added a commit to shijiesheng/cadence-java-client that referenced this pull request Sep 15, 2025
What changed?

added client entities that maps 1:1 with thrift (generated from a generator)
added BaseError, which is a catchall similar to TException.
added mappers to transform Proto to client entities. (mostly copied from thrift mappers)
added IWorkflowServiceV4 which is exactly same as IWorkflowService except for it's using the client entities.
added WorkflowServiceGrpc that implements the V4 interface
Plan

new V4 interface with new entities < This PR
replace all pointers to V4 in one go (Should be import change only)
Why?

Thrift deprecation work

How did you test it?

Unit Test on mappers from ClientObjects to ProtoObjects
shijiesheng added a commit to shijiesheng/cadence-java-client that referenced this pull request Sep 16, 2025
What changed?

added client entities that maps 1:1 with thrift (generated from a generator)
added BaseError, which is a catchall similar to TException.
added mappers to transform Proto to client entities. (mostly copied from thrift mappers)
added IWorkflowServiceV4 which is exactly same as IWorkflowService except for it's using the client entities.
added WorkflowServiceGrpc that implements the V4 interface
Plan

new V4 interface with new entities < This PR
replace all pointers to V4 in one go (Should be import change only)
Why?

Thrift deprecation work

How did you test it?

Unit Test on mappers from ClientObjects to ProtoObjects
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