Proposal for high level client APIs for best practices #99
Comments
|
This is great @allenxwang, thank you for kicking off this discussion with a well thought-out proposal. |
|
Regarding the What is the reason for not having just |
|
The For example, right now with it just returning a boolean all Ribbon can now do is throw a standard Exception of some kind to trigger failure. Shouldn't we either let the function throw an In other words, if we instead just had a callback such as A signature could be: public void onNetworkResponse(Func1<T response, R returnResponse> f);Then it could be used like this: onNetworkResponse(HttpClientResponse<ByteBuf> response -> {
if(response.getStatus().code() >= 500) {
throw MyCustomException("message here");
} else {
return response; // pass-thru (but could decorate or modify if wanted)
}
});I don't know the input and return types well enough to get this quite right, but what do you think of this kind of approach that is more generic and allows both success and error handling with increased flexibility? |
|
The signature of Func1< HystrixExecutableInfo<T>, Observable<T>>So I think it should just be called Note also how I change it from So I suggest either |
|
For caching there are 2 types of cache keys we need to support:
The first is nothing but a simple function that takes the input args and returns the cache key. |
|
Regarding public RibbonRequest<HystrixResponse<T>> withHystrixInfo();It seems this is overly precise when there could be other types of metadata we want to make available, including the HTTP response codes, headers, etc (even when type T is returned). How about: public RibbonRequest<RibbonResponse<T>> withMetadata();
public interface RibbonResonse<T> {
public HystrixExecutableInfo getHystrixInfo();
public HttpHeaders getHttpHeaders(); // or whatever we want here
... etc ...
public T getResponse();
} |
|
On the |
|
The I suggest we consider having two different object models, one for request/response like HTTP (including SSE), and another for bi-directional such as TCP and WebSockets. This is similar to the decisions we've made in RxNetty since request/response and bi-directional streaming are very different. How do you think that will affect this design? |
|
Thanks @benjchristensen for comments!
I agree, the AsyncRequest name does not represent what it does. The intent of having a base interface for RibbonRequest was that HystrixResponse interface also had the same methods execute(), queue(), observe() and toObservable() but the name is not correct. Also, we can just keep it package private and not expose it outside as the intent is just to share the methods.
We discussed about this point, but decided against it because the use case seems to be of transforming a response and it fits in naturally more at the transport level instead of the Ribbon API level. I was intending to move the karyon interceptor abstraction into rx-netty and make it available for both client and server. Finagle calls this abstraction as filters.
Agree with this comment entirely.
The reason why this does not fit in as two
Agree with the name being too precise.
They should both be named
The purpose of RibbonTransport is to marry ribbon's load balancing feature with RxNetty's transport. However, from this point I am starting to think that we can completely eliminate the RibbonClient abstraction. Instead we may want to only have public final class Ribbon {
private Ribbon() {
}
public static <I, O> HttpRequestTemplate<I, O> newHttpRequestTemplate(HttpClient<I, O> transportClient) {
// ...
}
public static <I, O, T> T from(Class<T> contract, HttpClient<I, O> transportClient) {
// ...
}
} |
|
@benjchristensen are the
|
|
Building on Ben's comments regarding: public RibbonRequest<HystrixResponse<T>> withHystrixInfo();becoming public RibbonRequest<RibbonResponse<T>> withMetadata();I Agree and believe it would be best not to name Hystrix by name at this API level. Just as we don't expose Netty vs. another transport, lets not expose Hystrix vs. another (as yet undefined) resiliency mechanism. |
|
Please don't loose the ability to create templates based on templates that is in the original RibbonResource prototype that I wrote. it is extremely useful when you get down to creating actual thin clients on top of this. |
|
Thanks to @NiteshKant for great design suggestions and @benjchristensen for the comments!
What about something like
We choose to have it return a simple Boolean under the assumption that it is simple enough for majority of use cases. I see the point that having a response transforming API in Ribbon can reduce the burden of client creator for creating his own transport client. But if the interceptor in rx-netty can make it simple then it might be a better option.
We thought about this. The reason it only returns Hystrix metadata is that at the point this information is available, Hystrix execution is already done and it is hardly useful to check the protocol specific meta data, which is the layer below Hystrix. Also it would be cumbersome from design point of view (especially generics) to include protocol specific data here.
That is my intention. Let me know if Hystrix cache should be reflected in the design in a different way.
Seems like a good idea. |
@khawes can you explain the usecase more? |
@allenxwang yeah that could work too but I would vote for making the interface package private as it is just for defining common methods and making it public introduces confusion as to why is there a RibbonRequest and RxRequest? What is the difference between them? |
|
Here's some sample code for being able to define a template using another template. Note the ResourceTemplateSettings differs from the ResourceTemplate class in that it has no generic return classes so that it can be reused in many templates: private final RESTClient restClient;
private static final ResourceTemplateSettings defaults = new ResourceTemplateSettings()
.withClientVersion(DMSClient.class)
.withRESTClientName(NIWSClientName)
.withErrorClass(RESTResult.class);
private static final ResourceTemplate<RendezvousEntity> POST_RendezvousStart =
new ResourceTemplate<RendezvousEntity>(RendezvousEntity.class,
RestClient.Verb.POST,
"/rendezvous",
defaults)
.withSuccessHttpStatus(HttpStatus.SC_CREATED)
.withErrorHttpStatus(HttpStatus.SC_CONFLICT,
HttpStatus.SC_UNAUTHORIZED);and later it's called like this: public Resource.Builder<RendezvousEntity> rendezvousStart(String esn) {
RendezvousEntity entity = new RendezvousEntity();
entity.esn = esn;
entity.state = DeviceEntity.State.Init;
return POST_RendezvousStart.resourceBuilder().withEntity(entity);
} |
|
What does "success status" and "error status" end up causing to occur? |
|
Success will cause the return of the RendezvousEntity.class in this -- Keith Don’t listen to me, I play with Christmas lights On Thu, Jun 12, 2014 at 10:52 AM, Ben Christensen notifications@github.com
|
|
How does this differ from just having a function that lets you do whatever is needed? onNetworkResponse(HttpClientResponse<ByteBuf> response -> {
if(response.getStatus().code() >= 500) {
throw MyCustomException("message here");
} else {
return response; // pass-thru (but could decorate or modify if wanted)
}
}); |
|
It does not. We evolved into that function. Fon't focus on the details of the example but on the use case for defining one template using another template, which contains a set of defaults for the overall client that would be tedious and error prone to copt into every resource template in your client. |
|
This discussion is about the details – we are designing the API :-)
Cool, that's good to know ... as we want to have something as generic as possible without making it useless.
Makes sense to make reuse easy, basically "partial application" (http://en.wikipedia.org/wiki/Partial_application and http://www.ibm.com/developerworks/library/j-jn9/). |
Does it translate to having a method copy() on RequestTemplate which creates a new copy and then since the builder is additive, you start building from the copy. |
Probably something like that as I doubt we can afford the object allocation overhead of making the builder immutable. If we could do it as an immutable builder (like Rx sequences) then you can take any Template at any point and just embed inside something else without worrying about mutation. |
|
I made a copy of the builder in the containing object and passed the get methods down to the default if there were no existing values (e.g. null). For headers they were merged on build. I don't recall my reasoning as it's been a few years now since I wrote it. |
|
Changes after the discussion reflected in commit: allenxwang@8b74aff Summary of changes:
|
|
@allenxwang can you update the design proposal in this issue with the latest code?
Passing a list would prohibit use of lambdas/closures. |
Yeah it would be good to have the template thread-safe (by immutability) but the cost would be large as it means every mutation copies the template. I think we can just say that the template is not threadsafe from the point of view of mutations to the template, however each |
|
@khawes If I understand correctly, you are fine with the approach of providing a |
|
Yes
|
|
@khawes You will have access to HystrixExecutionInfo to see if the empty result is from fallback. The network response (Http headers) are not exposed after Hystrix execution is done since it is hardly useful once the final result (after fallbacks) is available. |
|
Let me add context to the question that I though was obvious but apparently is not. From a user of my client library, who eventually calls get() to get the object he/she is expecting from the service being called. |
|
A |
|
that's what I was missing, and is also missing from your example above. Now back to our regularly scheduled debate: I believe that the most common use cases can be handled by class FallbackDeterminatorBuilder() {
.withSuccessHttpStatus(int... httpStatusCodes)
.withErrorHttpStatus(int... httpStatusCodes);
// or
.withNonFallbackHttpStatus(int... httpStatusCodes);
// take your pick
}which makes it even easier for adoption. |
|
Given comments from @benjchristensen, the choices to implement null/empty (in case of 404) are (in the ResponseTransformer):
|
I do not agree that providing this helper method will make the path to adoption of ribbon easier or less error prone. I do not think it's a mainstream usecase either. The approach of class FallbackDeterminatorBuilder() {
.withSuccessHttpStatus(int... httpStatusCodes)
.withErrorHttpStatus(int... httpStatusCodes);
// or
.withNonFallbackHttpStatus(int... httpStatusCodes);
// take your pick
}addresses specific use cases that we now think are reasonable. Anything out of these cases will require a different function. eg: I can not do a check based on a range of error codes (> 200, 400-500, etc.) |
|
@NiteshKant one can absolutely do range of error codes with the If I'm looking at using RibbonClient for the first time How do I know that fallback logic goes in some object that needs to implement RibbonClient should be simple and easy to use for most use cased yet be able to handle the more advanced cases by dropping down a layer. If you conflate these two layers you end up with a high barrier to entry, and teams writing the high level layer over and over, and we loose best practices again. |
|
Fundamentally, as a client writer I must always specify when to fallback. However as a client writer I cannot always specify how to fallback.
By combining the two disparate actions into one method it makes it very difficult to separate the two concerns. Those that cannot provide code for fallback will end up not providing for fallback, and users will not only have to provide fallback but also define when. We have this situation today because Hystrix already has this conflated view of falling back, and because of it we wound up with at least 3 separate implementations per DMS end point, and each and every one of them got it wrong on several occasions, and in addition others opted to not implement Hystrix at all. We need to keep determining when to fallback separate form the act of falling back; so that we can have users of the client easily specify fallback actions, while leaving when to fallback to the client writer. |
The function being discussed has little to do with Rx so please don't conflate them. The function is nothing more than a callback to process from |
The two concerns of when to provide fallback and the fallback itself are expressed as:
|
|
How does the The fallback, if you can implement one, is passed into the The two use cases you are referring to are handled in two different places. Here is the code again to show the two places: Ribbon.from(httpClient)
.newRequestTemplate()
.onNetworkResponse((HttpClientResponse<ByteBuf> response) -> {
// use case 1: when to fallback
// handle use case to determine a failure based on network or data
return ...;
})
.withFallback((HystrixExecutableInfo info) -> {
// use case 2: how to fallback
}) |
So that's not clear at all based on the name of the method
and
and
All of these say I need to to return T (my result object) so it looks like the fallback response object needs to be returned at this point. There is no contract between The result of determining fallback should be either:
not one of:
If you walk down this path you will find people handling the fallback internal to If you need to have something to mess with then by all means create a way to do so, but it shouldn't be related to fallback or its confusing. |
|
If I understand correctly your arguments are:
For 2, even if we split it into two functions, I don't see how that prevents misuse. For example, we could code it like this" Ribbon.from(httpClient)
.newRequestTemplate()
.throwIfBad((HttpClientResponse<ByteBuf> response) -> {
// throw an exception if bad
if(x) throw new RuntimeException(); // this will go to fallback
})
.transformIfNeeded((HttpClientResponse<ByteBuf> response) -> {
return response or transformed response;
})
.withFallback((HystrixExecutableInfo info) -> {
// use case 2: how to fallback
})The So, we can split them up, but I don't like doing so. The a) do nothing, it's fine If we split these it is purely arbitrary, artificially limiting, and less efficient (causing the response to be processed twice). Semantically Considering all of that, let's determine whether (d) is necessary. If it's not then this debate is moot as we don't need the ability to transform and return type The premise of (d) has been that some data comes off the wire and there is a reason to allow modifying it somehow. Thinking about this further though, I'm not sure that will work very well due to serialization and static typing. We can't mess with static types all that well. It could work with dictionaries (mapping keys for example), but that is a runtime decision by the user, not something that can be coded into this by the developer providing the client. So perhaps Ribbon.from(httpClient)
.newRequestTemplate()
.onResponse((HttpClientResponse<ByteBuf> response) -> {
// throw an exception if bad
if(x) throw new RuntimeException(); // this will go to fallback
})
.withFallback((HystrixExecutableInfo info) -> {
// use case 2: how to fallback
})and if that's the case, would a name like Ribbon.from(httpClient)
.newRequestTemplate()
.validateResponse((HttpClientResponse<ByteBuf> response) -> {
// throw an exception if bad
if(x) throw new RuntimeException(); // this will go to fallback
})
.withFallback((HystrixExecutableInfo info) -> {
// use case 2: how to fallback
})However, the thing I don't like about Should transforming of the response be permitted or not? It looks like it may be problematic. |
|
I just spent the last 45 mins catching up on this thread. I think at this point it may warrant an in person discussion on these points. I think we may run the risk of adding complexity to this design to offer too much flexibility. Rather than making this design fit more and more complex use cases, maybe we should question the validity of the use case. Should we ask ourselves if it's easier in terms of the design and reduction of complexity to have the server side absorb some of this complexity? For Keith's use case, I wonder if there is a flag that can be passed to the server to determine success vs failure. In general, I favor less specific methods that allow ease of understanding and use. |
|
Here are two examples: A/B: can specify when and how to fallback because fallback uses the test metadata for falling back. For these examples, these are great cases for flipping the logic; AB can have the test metadata passed in; for DMS requests, this data can be passed in to the client as well. This simplifies the logic within the client, and makes the caller need to explicitly pass in the appropriate data for the call. |
Which use case are we talking about? There is a face to face on Friday @11:30 |
|
@mikeycohen Flipping is a great way to explain what I'm trying to get across. Extending your example to it's logical conclusion we get:
|
|
A change to the design regarding API for get metadata (e.g. HystrixExecutableInfo): public interface RibbonRequest<T> {
public T execute();
public Future<T> queue();
public Observable<T> observe();
public Observable<T> toObservable();
public RequestWithMetaData<T> withMetadata();
}
public abstract class RibbonResponse<T> {
public abstract T content();
public abstract HystrixExecutableInfo<T> getHystrixInfo();
}
public interface RequestWithMetaData<T> {
Observable<RibbonResponse<Observable<T>>> observe();
Observable<RibbonResponse<Observable<T>>> toObservable();
Future<RibbonResponse<T>> queue();
RibbonResponse<T> execute();
}With this change, one is possible to do request.withMetadata().observe()
.flatMap(new Func1<RibbonResponse<Observable<ByteBuf>>, Observable<String>>() {
@Override
public Observable<String> call(RibbonResponse<Observable<ByteBuf>> t1) {
if (t1.getHystrixInfo().isResponseFromFallback()) {
return Observable.empty();
}
return t1.content().map(new Func1<ByteBuf, String>(){
@Override
public String call(ByteBuf t1) {
return t1.toString();
}
});
}
}); |
I think response transformation belong to the transport layer. Once it comes to the ribbon API layer, there should not be any transport response change done (atleast not evidently from the API). So in this case, instead of |
|
Latest design is in gist: https://gist.github.com/allenxwang/9f290dc863705bb4903f Changes:
|
|
|
@NiteshKant the name As for making it a first class interface... I believe that it needs to be. We want to be encouraging the best practice of specifying when fallback is necessary and The builder pattern is great for calling this out. All that aside I think we are diverging from our original intention of having clients built on top of this that are so thin we can almost "test" them with our eyeballs. e.g. code so simple it's hard to introduce non-obvious errors. Are we adding functionality that is useful for a few edge cases? I think we are. Take a step back an look at NIWSRestClient. It was very flexible and required a lot of boilerplate be created by client libraries for the common use cases. So much so that we have rest-client-utils that wraps it to allow all that boilerplate to be pushed down a layer. The result is straight forward to use and difficult to get wrong. It is also more restrictive, but that's how it gets it's simplicity. If you need the flexibility you can always drop down a layer and have at it. That's a long winded way of saying we need move the edge cases down a layer? I like to think about the common client from the client library users perspective. And think it should look like this: SubscribedObservable<DmsDevice> deviceCall = dmsClient.getCustomerDevice(esn,
customerid,
new myFallbackhandler(cTicket));if fallback must be provided by the user and: SubscribedObservable<DmsDevice> deviceCall = dmsClient.getCustomerDevice(esn, customerid);if not. Don't dump all over the word Oh and don't make me type In my head the implementation of ` dmsClient.getCustomerDevice(esn, customerId, myFallbackhandler(cTicket));' looks something like this: SubscribedObservable<DmsDevice> deviceCall(String esn, Long customerId, FallbackHandler fallback) {
GetCustomerDeviceRequestTemplate.getResource("esn", esn, "custId", customerId)
.withResponseValidator(deviceCallValidator)
.withFallback(fallbackHandler)
.build() // the act of building fires off the remote call (subscribes)
}
}Don't make me type
Finally, if as a user if I feel I need to get at the builder before it's built I can go talk to the client library owner to have a method exposing the builder, or if the owner sees benefit for all their users it gets built into the client and everybody gets the enhanced functionality. End of line. |
|
The design is updated here
The reason why |
|
PS Milliseconds is one word so no need to camel case the S in seconds. |
|
in |
|
Minor naming changes in |
|
After discussion with @mikeycohen, the |
|
Close this as the design is reflected in new ribbon module in 2.0-RC1. New issues will be created for further improvements. |
Note: the latest design is here
Goals
In the past two years, Ribbon has been used as an inter-processing communication client with load balancing capabilities. Some best practices have been observed when using Ribbon together with other open source libraries. We feel it is best to offer such best practices in a form of a set of APIs in a new Ribbon module.
Here is that we want to achieve:
The ultimate goal is that the clients created with this set of API should be more consistent, agile, reliable, efficient, and resilient.
Design proposal
The center pieces of the new API are
AsyncRequestandRibbonRequestRequestTemplateis used to represent all the information, including cache providers, Hystrix fallback and any protocol specific information (e.g. URI for HTTP) required to createRibbonRequest. Information supplied to create theRequestTemplatemay include variables which should be substituted with real values at the time of request creation.FallbackDeterminatoris used to determine whether a response with protocol specific meta data should cause throw an error during Hystrix command execution and cause a fallback:For example:
Once
FallbackDeterminatordetermines that a protocol specific response should trigger Hystrix fallback, HystrixFallbackProvider is used to get the actual fallback:CacheProvideris the interface to provide access to cache of the resourceRibbonis the factory that createsThe ribbon-transport module in Ribbon will provide clients used by
Ribbonfor different protocol with load balancing capabilities. You can also use any client created directly RxNetty if load balancing is not required.The
RibbonTransportin ribbon-transport module is the factory to create load balancing clients:The text was updated successfully, but these errors were encountered: