[controller] Client side foundations and refactor to support gRPC transport for controller APIs

[mynameborat]

Summary, imperative, start upper case, don't end with a period

• Introduces new controller transport client for gRPC
• Introduce interfaces and models for writing new controller APIs / migrating existing ones to use gRPC transport
• Demonstrate the foundations and patterns with createStore API

How was this PR tested?

Unit tested

Does this PR introduce any user-facing changes?

• No. You can skip the rest of this section.
• Yes. Make sure to explain your proposed changes and call out the behavior change.

[FelixGV]

Left some comments, which may be a bit superficial in nature. I don't have a solid end-to-end view of the whole work, so maybe some of my suggestions don't make sense. Please take a look and LMK.

Thanks for this work! Looking forward to it landing!

[FelixGV]

The GrpcConvertersRegistry has both request and response converters. Does this one likewise need to have both?

[mynameborat]

Our current response classes are in a much better shape due to its decoupling from the transport details. It was only our request that had a strong coupling the underlying transport to begin with and hence the need to introduce a request abstraction that is purely driven and interacted by the controller client.

As a result, the current http flow already returns this response abstraction defined by the controller (ControllerResponse and its extensions); So I decided to piggy back on that abstraction and have GRPC ones convert their responses into the controller abstracted response.

Additionally, I didn't want to refactor the deserialization handling piece within the ControllerHttpTransport and even if we want to extract it, each ones don't need a concrete converter as the jackson way of deserializing things already converts the response into concrete ControllerReponse objects.

Let me know if that answers your question.

[FelixGV]

I guess I just don't understand why we need it for one but not the other...?

[mynameborat]

that is because, we decided to use a separate response data model for gRPC and ControllerResponse POJO isn't suitable, unless, we loose the typing information on the gRPC path and have responses represent byte[] in which case we can reuse all of it;

We decided as a team to follow gRPC convention and make the new request, response strongly typed.

[FelixGV]

What do T and D stand for? Although it is fine to use single letters for the generic type names, I think it's important to try to make these names as self-explanatory as possible. For example, I/O which stand for Input/Output might be fine. It is also fine to spell out whole words instead of using single letters, e.g. SOURCE/DESTINATION, or SRC/DEST.

[mynameborat]

T -> The controller request type (e.g., storeCreateRequest, jobStatusRequest)
D -> controller transport specific request type (e.g., http, grpc etc)

I did use some of elaborative naming for generic types in few places in the code where I saw no ambiguity. I thought a bit and then wasn't fully convinced if SRC and DEST was going convey its intent; I even wondered if SRCREQ to DESTREQ would help.

Given this converter interface is not a general converter interface rather specific to the controller requests, I found the above choices too generic and hence resorted write the parameter java document to provide some clarity. Do we have any convention on naming generic types within our code base?

For e.g., Can we have something like ControllerRequestType TransportRequestType?

[FelixGV]

T usually stands for "Type", which is fine if there is just a single thing to maintain a generic "type" for. In this case there are two things, however, so T seems ambiguous. D might stand for "Destination" in this context, though I'm not sure?

[mynameborat]

ah i see; T is your concern as it by default has the type connotation. Let me update ControllerRequestType and TransportRequestType if you have no objections. If not, I can fallback to SRC and DEST which i think is okay along with the java docs

[FelixGV]

What is the relationship between this class and ControllerRequest? Should this one extend the other?

[mynameborat]

ControllerRequest abstraction is meant to decouple the actual transport related information. ControllerHttpRequest however, describes how ControllerRequest will look like if http was chosen as the medium of transport.

Originally, I thought about having some coupling between ControllerRequest, ControllerHttpRequest and GeneratedV3.. (grpc general request type)in the form of having ControllerRequest host few abstract methods like

  public abstract ControllerRequest {
      ...
      abstract ControllerHttpRequest convertToHttpRequest();

      abstract <T extends GeneratedV3..> T convertToGrpcRequest();
 }

However, the above spills some of the transport details into the model and hence chose against it;

[FelixGV]

Must we use reflection? Is there not a design pattern that could allow us to plug in the correct stuff without it? e.g. perhaps this "stub" class can return a Function<REQ, RES>, or similar?

[mynameborat]

Good point. Of the 5 days in the hackathon, I spent considerable amount of time to make this generic instead of using reflection. Most of my attempts failed due to type erasure and the need to introduce type token to carry information in a way it could potentially work.

That said, GRPC way of defining these methods are also sort of either too abstracted or too narrow; e.g., the stub generated with createStore method takes a concrete CreateStoreRequest so having type bounds on REQ and RES to work seemed a bit more tricky and hence I chose this one for now.

I will continue looking into this and see if TypeToken is the way to go and perhaps refactor this out if I land on something promising.

[FelixGV]

Ok well, this is not a hot path so the performance downside of reflection is not a concern... the only concern is the maintainability aspect since we lost strong typing. I guess it could be fine to let this in as is, and keep iterating later.

[FelixGV]

Should we include controller in the package name? Either in the last or second to last position?

[mynameborat]

should be fine; just need to coordinate the change w/ @sushantmane

[FelixGV]

We are going to have client/server (or "data plane") protocols as well. It would seem cleaner to me to have a clear separation between control plane and data plane protocols.

[FelixGV]

One more idea/suggestion about this...

[FelixGV]

Can this code be a function of CreateStoreRequest? Perhaps the parent class ControllerRequest can become ControllerRequest<GRPC> and define a public abstract GRPC getGRPC() API. Then CreateStoreRequest extends ControllerRequest<CreateStoreGrpcRequest>? Would that eliminate the need for the whole registry?

[mynameborat]

This goes back to my earlier comment - #1259 (comment)

Controller models (responses and requests) taking dependencies on the underlying transport through methods like toGrpc and fromGrpc would make the consumers of these models exposed to some underlying details (although we could control the access modifier) this also potential brings in the dependencies of the underlying transport dependencies for consumers of models.

Hence the choice to invert the dependency and have these concrete transport implementations depend on the models of controller abstraction and the conversion residing outside these models.

[FelixGV]

And similarly, the function to convert into a ControllerHttpRequest could likewise live over there?

[FelixGV]

And similarly to the idea I mentioned in my last review... can this become a function of NewStoreResponse? We could have ControllerResponse<GRPC, R extends ControllerResponse >, with NewStoreResponse extends ControllerResponse<CreateStoreGrpcResponse, NewStoreResponse>? Although the API would be the reverse, e.g. public R fromGRPC(GRPC) {} or similar?