#1287 Docs: Example of custom aggregator #1330
Conversation
raman-m
changed the title
raman-m
force-pushed
the
docs/defined-aggregator-update
branch
from
6765365 to
9b62d30
Compare
raman-m
changed the title
raman-m
added
bug
| @@ -108,6 +108,31 @@ In order to make an Aggregator you must implement this interface. | |||
|
|
|||
| With this feature you can pretty much do whatever you want because the HttpContext objects contain the results of all the aggregate requests. Please note if the HttpClient throws an exception when making a request to a Route in the aggregate then you will not get a HttpContext for it but you would for any that succeed. If it does throw an exception this will be logged. | |||
There was a problem hiding this comment.
It is not clear, in case of downstream service failure, will we be able to process a HttpContext object in the Aggregate method?
Not an issue, just the question.
| { | ||
| var responses = responseHttpContexts.Select(x => x.Items.DownstreamResponse()); | ||
|
|
||
| var statusCodes = responses.Select(x => x.StatusCode); |
There was a problem hiding this comment.
Do we see non-20x status codes here, including 404, 50x, etc.?
So, Can we process failed responses in the method?
|
|
||
| return new DownstreamResponse( | ||
| new StringContent(JsonConvert.SerializeObject(contentList)), | ||
| HttpStatusCode.OK, |
There was a problem hiding this comment.
🆗 It seems we return HttpStatusCode.OK status even all downstream services have failed...
Is that correct?
| var responses = responseHttpContexts.Select(x => x.Items.DownstreamResponse()); | ||
|
|
||
| var statusCodes = responses.Select(x => x.StatusCode); | ||
| var headers = responses.SelectMany(x => x.Headers).ToList(); |
There was a problem hiding this comment.
Every downstream service response has own headers. In most of all services the headers can have the same names.
This simple SelectMany selector initializes one unsorted list of Header objects with a potential duplicated header names.
How to recognize that a header belongs to a particular service?
| new StringContent(JsonConvert.SerializeObject(contentList)), | ||
| HttpStatusCode.OK, | ||
| headers, | ||
| "reason"); |
There was a problem hiding this comment.
There is no merging of reasons from DownstreamResponse objects (aka ReasonPhrase string property).
We need to merge them somehow.
Simple returning of "reason" string is wrong!
| foreach (var response in responses) | ||
| { | ||
| var content = await response.Content.ReadAsStringAsync(); | ||
| contentList.Add(content); | ||
| } |
There was a problem hiding this comment.
We process the responses content sequentially. And the reading operation is asynchronous.
Why not to make the processing parallel?
Tasks.WhenAll(tasks) should help.
| } | ||
|
|
||
| return new DownstreamResponse( | ||
| new StringContent(JsonConvert.SerializeObject(contentList)), |
There was a problem hiding this comment.
For example Laura-key route has failed.
Do we return empty JSON-object {} or it will be absent at all?
{
"Tom": {"Age": 19},
"Laura": {}
}| @@ -108,6 +108,31 @@ In order to make an Aggregator you must implement this interface. | |||
|
|
|||
| With this feature you can pretty much do whatever you want because the HttpContext objects contain the results of all the aggregate requests. Please note if the HttpClient throws an exception when making a request to a Route in the aggregate then you will not get a HttpContext for it but you would for any that succeed. If it does throw an exception this will be logged. | |||
|
|
|||
| .. code-block:: csharp | |||
There was a problem hiding this comment.
There is no description of this sample!
Developers need to see some instructions and best design approaches while implementing a custom aggregator class.
| public class MyAggregator : IDefinedAggregator | ||
| { | ||
| public async Task<DownstreamResponse> Aggregate(List<HttpContext> responseHttpContexts) |
There was a problem hiding this comment.
There is no constructor definition!
It is better to inject common services including a IOcelotLoggerFactory logger.
|
@MJeorrett As an author of the issue #1287, What I see, as developer you were confused during implementation of your custom aggregate, because the docs lacks instructions how to best to build the aggregate. |
raman-m
requested review from
binarymash,
jps,
geffzhang,
RaynaldM,
philproctor,
anktsrkr,
sharpn,
abooraja,
wast,
yangzhongke and
thiagoloureiro
|
@jlukawska @raman-m Working on the Multiplexer refactoring, I realized this example is great ;-) Thanks! |
|
@ggnaegi Should we prioritize this PR because of changes in multiplexer and add it to the current release? |
|
@raman-m I have used the examples from @jlukawska in the Multiplexer documentation, so this PR could be closed. |
…1826) * Cleanup of Multiplexing middleware, avoiding creating copies of the Http context if only one downstream route. * updating comments * preparing rebase, it's a hack. * fixing test case "Copy_User_ToTarget", method name has changed. * adding some unit tests, checking that if 1 route, ProcessSingleRoute is called and no copies of context are made, if more than 2 Map is called, then more than 2 copies are created. * Applying refactoring suggested. * some code cleanup * Some code cleanup in Aggregate Logic * Cleanup in Aggregate Tests, verifying multiplexer cleanup * Finalizing unit test, with complex keys. * some code refactoring and applying suggestions from reviews * Update requestaggregation.rst Recover docs * updating docs * Update requestaggregation.rst * Update requestaggregation.rst * Code review by @raman-m * Inherit `Steps` functionality instead of private aggregation --------- Co-authored-by: Raman Maksimchuk <[email protected]>
|
@jlukawska Hi J.! |
Fixes #1287
Now it's not obvious how to access the response details in a custom aggregator.
Proposed Changes