aggregate cluster circuit breaker integration tests#35
aggregate cluster circuit breaker integration tests#35
Conversation
Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>
Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>
Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>
nahratzah
left a comment
nahratzah
left a comment
There was a problem hiding this comment.
Left some comments for some small nits I have.
Feel free to accept them or reject them, they're really small things. <3
This is REALLY GOOD and READABLE! Love it. :)
|
|
||
| reduceAggregateClustersListToOneCluster(*aggregate_cluster); | ||
| setCircuitBreakerLimits(*aggregate_cluster, CircuitBreakerLimits{.max_connections = 1}); | ||
| setMaxConcurrentStreams(*aggregate_cluster, 1U); |
There was a problem hiding this comment.
It's fine, but I don't think you need the U in 1U, because the compiler should be able to figure that out on its own in this context. :)
(But you can keep it if you like it, or find out I'm wrong.) <3
| initialize(); | ||
|
|
||
| setCircuitBreakerLimits(cluster1_, CircuitBreakerLimits{.max_connections = 1}); | ||
| setMaxConcurrentStreams(cluster1_, 1U); |
There was a problem hiding this comment.
Similarly, the compiler should be able to figure out the U bit on its own, so it's not required.
(But you can keep it if you like it, or find out I'm wrong.) <3
| reduceAggregateClustersListToOneCluster(*aggregate_cluster); | ||
| setCircuitBreakerLimits(*aggregate_cluster, CircuitBreakerLimits{.max_connections = 1}); | ||
| setMaxConcurrentStreams(*aggregate_cluster, 1U); |
There was a problem hiding this comment.
Just want to mention I love how easy it is to understand what these functions do! ❤️
| test_server_->waitForCounterEq("cluster.cluster_1.upstream_cx_overflow", 1); | ||
|
|
||
| // the requests to the aggregate cluster route affect the cluster1 circuit breaker state | ||
| // send a 3rd request directly to cluster1 |
There was a problem hiding this comment.
minor nit, feel free to ignore if you don't like it. <3
| // send a 3rd request directly to cluster1 | |
| // send a 3rd request directly to cluster1 to confirm this |
| test_server_->waitForCounterGe("cluster.cluster_1.upstream_cx_overflow", 2); | ||
| // the overflow may be greater than 2 because after completing the first request | ||
| // the queued pending requests will attempt to reuse the connection |
There was a problem hiding this comment.
I think it's custom to put the why-comment before the statement, not after. :)
| test_server_->waitForCounterGe("cluster.cluster_1.upstream_cx_overflow", 2); | |
| // the overflow may be greater than 2 because after completing the first request | |
| // the queued pending requests will attempt to reuse the connection | |
| // the overflow may be greater than 2 because after completing the first request | |
| // the queued pending requests will attempt to reuse the connection | |
| test_server_->waitForCounterGe("cluster.cluster_1.upstream_cx_overflow", 2); |
but if you dislike that, I'm fine with you keeping this. <3
| third_request_retry.encodeHeaders(default_response_headers_, true); | ||
| ASSERT_TRUE(cluster1_response1->waitForEndStream()); | ||
| EXPECT_EQ("200", cluster1_response1->headers().getStatusValue()); | ||
| // respond to the fourth request to cluster1 |
There was a problem hiding this comment.
This isn't a response I think? It's just the request failing, so it's more like "handle the response" instead of responding. <3
| first_request_retry.encodeHeaders(default_response_headers_, true); | ||
| ASSERT_TRUE(aggregate_cluster_response1->waitForEndStream()); | ||
| EXPECT_EQ("200", aggregate_cluster_response1->headers().getStatusValue()); | ||
| // respond to the second request to the aggregate cluster |
There was a problem hiding this comment.
This isn't a response I think? It's just the request failing, so it's more like "handle the response" instead of responding. <3
Co-authored-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com> Signed-off-by: Baz Murphy <61154071+bazmurphy@users.noreply.github.com> Signed-off-by: Saadia El fekak <74792703+SaadiaELF@users.noreply.github.com>
2 participants
The version before we add it to the upstream PR