test: deflaking */LoadStatsIntegrationTest.LocalityWeighted/*#7225
test: deflaking */LoadStatsIntegrationTest.LocalityWeighted/*#7225alyssawilk merged 7 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Thanks, LGTM, nice catches. A few minor comments and we can merge.
/wait
test/test_common/utility.h
Outdated
| static std::vector<std::string> split(const std::string& source, const std::string& split, | ||
| bool keep_empty_string = false); | ||
|
|
||
| /** |
There was a problem hiding this comment.
envoy/source/common/protobuf/utility.h
Line 144 in 3e0ee3c
There was a problem hiding this comment.
I can use that, but to avoid the removal getting really expensive I'd have to iterate from the back. I think that makes the code harder enough to read I'd prefer to leave as-is unless you have strong feelings about it?
There was a problem hiding this comment.
The template could probably easily be extended to have other container types if you are concerned about the list vs. vector ^^.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
source/common/protobuf/utility.h
Outdated
| template <typename ProtoType, typename ReturnType> | ||
| static ReturnType | ||
| convertToConstMessagePtrContainer(const Protobuf::RepeatedPtrField<ProtoType>& repeated_field) { | ||
| ReturnType ret_vector; |
There was a problem hiding this comment.
Maybe rename the variable now that it's not always a vector?
| @@ -461,22 +478,16 @@ TEST_P(LoadStatsIntegrationTest, LocalityWeighted) { | |||
| initialize(); | |||
|
|
|||
| // Debug logs for #6874 | |||
test/test_common/utility.h
Outdated
| * | ||
| * @param lhs RepeatedPtrField on LHS. | ||
| * @param rhs RepeatedPtrField on RHS. | ||
| * @param ignore_repeated_field_ordering if ordering should be ignored. Note if true this turns |
There was a problem hiding this comment.
Name here doesn't match the paramter name
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
3 participants
For posterity #6875 got us the information it was a "waiting for stats" bug, which is then debuggable by lowering load stats interval to 1ms.
This exposed two issues with merge. When request 1 arrived in a separate interval from request 2, the stats protos could arrive in the opposite order (fixed by ignoring order) and that if there was a stats-update with a request in progress it never resolved because we were always adding fields (oops!)
I'm tempted to leave the interval low now that this is thoroughly debugged, but there's expectations on the time being within a range which wouldn't work if we lower it so meh?
Risk Level: n/a (test only)
Testing: yes. very much so.
Docs Changes: n/a
Release Notes: n/a
Fixes #6874