Closed ggnaegi closed 8 months ago
🤯 😮 Not now, not this month, Gui! 😄
nice job !
@ggnaegi
Should Process the downstream Route directly, without parallelization logic if 1 Route
But Multiplexing (Aggregation) should not be done for definitions with one route included only. Is this PR about this case only? So isn't any validation now for this border case? Can we define Aggregation which consists of one route only?
Seems this PR is small improvement (from feature point of view) because it touches design refactoring only...
@RaynaldM @ggnaegi Guys, if you want to prioritize this PR and delivery let me know plz. I have no idea how we will deliver... and what release to include this improvement?!...
@ggnaegi
Should Process the downstream Route directly, without parallelization logic if 1 Route
But Multiplexing (Aggregation) should not be done for definitions with one route included only. Is this PR about this case only? So isn't any validation now for this border case? Can we define Aggregation which consists of one route only?
Not only, the code refactoring and readability improvement are important too, but by design the logic is handling definitions with one downstream route only, the same way it would handle a route with multiple downstream routes:
var routeKeysConfigs = httpContext.Items.DownstreamRouteHolder().Route.DownstreamRouteConfig;
if (routeKeysConfigs == null || !routeKeysConfigs.Any())
{
var downstreamRouteHolder = httpContext.Items.DownstreamRouteHolder();
var tasks = new Task<HttpContext>[downstreamRouteHolder.Route.DownstreamRoute.Count];
for (var i = 0; i < downstreamRouteHolder.Route.DownstreamRoute.Count; i++)
{
var newHttpContext = Copy(httpContext);
newHttpContext.Items
.Add("RequestId", httpContext.Items["RequestId"]);
newHttpContext.Items
.SetIInternalConfiguration(httpContext.Items.IInternalConfiguration());
newHttpContext.Items
.UpsertTemplatePlaceholderNameAndValues(httpContext.Items.TemplatePlaceholderNameAndValues());
newHttpContext.Items
.UpsertDownstreamRoute(downstreamRouteHolder.Route.DownstreamRoute[i]);
tasks[i] = Fire(newHttpContext, _next);
}
await Task.WhenAll(tasks);
Seems this PR is small improvement (from feature point of view) because it touches design refactoring only...
Yes, it's a small improvement, but it will allow further improvements since a contributor can now easily understand what's achieved here.
@RaynaldM @ggnaegi Guys, if you want to prioritize this PR and delivery let me know plz. I have no idea how we will deliver... and what release to include this improvement?!...
December? It's not such a big deal, I can add a few more unit tests.
Lack of unit tests! Add acceptance tests too, or rework current ones.
Yes I wrote it somewhere. It was a kind of draft 🤣
But the unit tests for the middleware and the acceptance are already written.
You 've done significant refactoring but you're joking... Cannot understand that!... For sure I see that current tests are passed, but your idea which implemented here is not tested.
You can reuse phrases from Proposed Changes as names of future tests.
As I said, it's a draft, I'm working on the test cases. I will let you know when i'm ready.
@raman-m Should be part of January 24 Release. Not much work left, only the unit tests.
@ggnaegi Are you sure adding it to Jan'24? Tomorrow is the last day of the milestone. But we will not release tomorrow for sure: a lot of tasks to close... OK No problem I'm going to add to the current release...
@ggnaegi Let me know once you've finished developing it and I'll review.
During refactoring pay attention to the 2nd else
-branch of the large if
-block of the Invoke
method. This is undocumented feature of parsing data for placeholders and/or parsing extra JSON configs for placeholders logic, aka "Advanced Aggregation".
But I expect a lot of design challenges in this block. And, could you dive deeply into this logic please to understand this Advanced Aggregation feature please? It would be better to update our docs about this feature.
https://github.com/ThreeMammals/Ocelot/blob/108bedef7a64698063230b2d911e382d57cc3a60/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs#L76-L158
Ocelot wishes "Good luck!" 🐯
@raman-m please review, I have modified the docs too. It seems the functionality "advanced request aggregation" is still in its early stages. Whether or not this functionality should be still maintained in the future is not part of the PR.
Will this PR close #1287 ?
Closes #1825 #1287 #1069
1825
1287
1069
Includes #1330
1330
Proposed Changes
Some benchmarks (based on develop branch 30.11.2023): Before
After
We have a performance increase of around 6% if only 1 downstream route.
@raman-m @RaynaldM