feat (thrift deprecation)!: replace thrift entities with internal ones #1022
Conversation
shijiesheng
requested review from
Groxx,
abhishekj720,
natemort,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
shijiesheng
force-pushed
the
v4.x.x
branch
2 times, most recently
from
f240cbb to
c7070b3
Compare
shijiesheng
changed the title
Signed-off-by: Shijie Sheng <[email protected]>
|
|
||
| if (response.getExecutionsSize() < listRequest.getMaximumPageSize()) { | ||
| int neededPageSize = listRequest.getMaximumPageSize() - response.getExecutionsSize(); | ||
| if (response.getExecutions().size() < listRequest.getMaximumPageSize()) { |
There was a problem hiding this comment.
if getExecutions() returns null this would cause a NPE. Not sure if that's actually possible based on the mapper and how protos behave.
There was a problem hiding this comment.
addressed similar cases with default initialization in the generated entity. For example,
@data
class SomeEntity {
List<String> SomeList = new ArrayList<>();
}
| import org.apache.thrift.TException; | ||
| import org.apache.thrift.async.AsyncMethodCallback; | ||
|
|
||
| public interface IWorkflowService extends Iface, AsyncIface { |
There was a problem hiding this comment.
Let's remove IFace and AsyncIface. I think IWorkflowService works well enough as the main interface and none of our code refers to either of these explicitly. It's unlikely that user code does as well.
| CadenceError; | ||
| } | ||
|
|
||
| interface AsyncIface { |
There was a problem hiding this comment.
Can we remove any of the methods in AsyncIface that were never implemented? I recall there being a bunch that will just throw an exception. Users are unlikely to be calling them since they never worked in the first place.
There was a problem hiding this comment.
Huh it turns out they're pretty much all implemented in gRPC. I guess our gRPC support was more thorough than Thrift.
There was a problem hiding this comment.
Yeah, I'll just leave it unchanged.
| return nullFields; | ||
| } | ||
|
|
||
| public static <E extends Enum<E> & TFieldIdEnum, M extends TBase<M, E>> void assertMissingFields( |
There was a problem hiding this comment.
We should be able to remove the thrift functions from here.
There was a problem hiding this comment.
removed all thrift related stuff
| RequestMapper::registerDomainRequest, | ||
| "emitMetric"), // Thrift has this field but proto doens't have it | ||
| RequestMapper | ||
| ::registerDomainRequest), // Thrift has this field but proto doens't have it |
There was a problem hiding this comment.
Nit: Either the comment was incorrect or something weird is going on. Something should be changed here (maybe we just need to remove the comment).
There was a problem hiding this comment.
I've removed the comment
| compile group: 'javax.annotation', name: 'javax.annotation-api', version: '1.3.2' | ||
| compile group: 'com.auth0', name: 'java-jwt', version:'3.10.2' | ||
| compile group: 'com.google.protobuf', name: 'protobuf-java', version: '3.21.9' | ||
| compile group: 'com.google.api.grpc', name: 'proto-google-common-protos', version: '2.10.0' |
There was a problem hiding this comment.
I can't comment on the specific line (github won't let me), but we should remove the tchannel and libthrift dependencies that are above here.
There was a problem hiding this comment.
Also fine if that's a followup PR to remove the lingering references. It seems like thrift library types are still used here and there.
There was a problem hiding this comment.
I've cleaned up all thrift related logic.
What changed?
Migration Guide
new WorkflowServiceGrpc()instead ofnew Thrift2ProtoAdapter(IGrpcServiceStubs.newInstance());TExceptionand now all workflow service method returnsCadenceErrorinstead;Why?
Java thrift deprecation
How did you test it?
Unit Test
Potential risks
Release notes
Documentation Changes