Replace std::unordered_map/set with absl containers by sunjayBhatia · Pull Request #11879 · envoyproxy/envoy

sunjayBhatia · 2020-07-03T06:00:59Z

Commit Message:
Replace std::unordered_map/set with absl containers

Replace with absl::node_hash_map/set
- Primarily for performance optimizations and to root out any
  assumptions made about iteration order in tests or otherwise (the
  replacement absl containers have a non-deterministic iteration
  order
- absl::node_hash_map/set should be drop-in replacements for
  std::unordered_map/set
Note that a future refactor should reevaluate and move to
absl::flat_hash_map/set where possible for memory optimizations
Add format check to disallow future usage of std::unordered_map/set
Small changes made where absl containers required it or tests needed
to be modified for correctness

Additional Description:

There may be an issue we should open with abseil about emplace and try_emplace when attempting to do in-place construction
- When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees)
- //test/server:overload_manager_impl_test demonstrates this
- see flat_hash_map::try_emplace() is not exception safe abseil/abseil-cpp#388

Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally
Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang
Docs Changes: N/A
Release Notes: N/A
Fixes #11825

- Replace with absl::node_hash_map/set - Primarily for performance optimizations and to root out any assumptions made about iteration order in tests or otherwise (the replacement absl containers have a non-deterministic iteration order - absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set - Note that a future refactor should reevaluate and move to absl::flat_hash_map/set where possible for memory optimizations - Add format check to disallow future usage of std::unordered_map/set - Small changes made where absl containers required it or tests needed to be modified for correctness Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

- Reviewing all unordered_map, we discovered this test was flawed, because it assumed ordering of map elements. This is true of libc++ and msvc, but not consistant with one another, and not true absl::node_hash_map at all, resulting in test failing 50% of the time. - Reviewing the RFC, the hash comparison itself did not follow json. - Note this could be updated with the equivilant absl collection type if and when all std::map's are refactored. Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

test now passes on Windows Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

wrowe · 2020-07-06T15:21:48Z

Included in 11825 as well, but this should be ready to approve and merge, just waiting on retest of an unrelated asan flake.

wrowe · 2020-07-06T15:23:25Z

Included in 11825 as well, but this should be ready to approve and merge, just waiting on retest of an unrelated asan flake.

Correction, I transposed the test PR ID's. It is 11875 which should be ready to approve, this one has a bit more WIP.

antoniovicente · 2020-07-06T17:34:30Z

Notes:

Depends on Use std::map for computing json object hash #11875, temporarily cherry picks commit from that PR to get past format check

There may be an issue we should open with abseil about emplace and try_emplace when attempting to do in-place construction

cc: @ahedberg
Did you file an issue at https://github.com/abseil/abseil-cpp/issues about the emplace exception issue?

When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees)

//test/server:overload_manager_impl_test demonstrates this

Commit Message:
Replace std::unordered_map/set with absl containers

Replace with absl::node_hash_map/set

Primarily for performance optimizations and to root out any
assumptions made about iteration order in tests or otherwise (the
replacement absl containers have a non-deterministic iteration
order

absl::node_hash_map/set should be drop-in replacements for
std::unordered_map/set

Note that a future refactor should reevaluate and move to
absl::flat_hash_map/set where possible for memory optimizations

Add format check to disallow future usage of std::unordered_map/set

Small changes made where absl containers required it or tests needed
to be modified for correctness

Additional Description:
Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally
Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang
Docs Changes: N/A
Release Notes: N/A
Fixes #11825

ahedberg · 2020-07-06T17:42:10Z

Notes:

Depends on Use std::map for computing json object hash #11875, temporarily cherry picks commit from that PR to get past format check

There may be an issue we should open with abseil about emplace and try_emplace when attempting to do in-place construction

cc: @ahedberg
Did you file an issue at https://github.com/abseil/abseil-cpp/issues about the emplace exception issue?

abseil/abseil-cpp#388 may be relevant.

When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees)

//test/server:overload_manager_impl_test demonstrates this

Commit Message:
Replace std::unordered_map/set with absl containers

Replace with absl::node_hash_map/set

Primarily for performance optimizations and to root out any
assumptions made about iteration order in tests or otherwise (the
replacement absl containers have a non-deterministic iteration
order

absl::node_hash_map/set should be drop-in replacements for
std::unordered_map/set

Note that a future refactor should reevaluate and move to
absl::flat_hash_map/set where possible for memory optimizations

Add format check to disallow future usage of std::unordered_map/set

Small changes made where absl containers required it or tests needed
to be modified for correctness

Additional Description:
Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally
Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang
Docs Changes: N/A
Release Notes: N/A
Fixes #11825

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

antoniovicente

Thanks for taking on this cleanup.

include/envoy/grpc/status.h

antoniovicente · 2020-07-06T19:18:05Z

include/envoy/upstream/cluster_manager.h

  virtual ClusterInfoMap clusters() PURE;

-  using ClusterSet = std::unordered_set<std::string>;
+  using ClusterSet = absl::node_hash_set<std::string>;


This change may be related to memory usage increases detected by //test/integration:stats_integration_test

[ RUN ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithFakeSymbolTable/IPv4
test/integration/stats_integration_test.cc:290: Failure
Expected equality of these values:
m_per_cluster
Which is: 44597
44491

I wonder how the flat_hash_map/flat_hash_set variants compare.

pushed a change to move this to flat_hash_set, will see how this performs

[ RUN ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithFakeSymbolTable/IPv4 test/integration/stats_integration_test.cc:304: Failure Expected equality of these values: m_per_cluster Which is: 44842 45003 Stack trace: 0x8f6b99: Envoy::(anonymous namespace)::ClusterMemoryTestRunner_MemoryLargeClusterSizeWithFakeSymbolTable_Test::TestBody() 0x174a378: testing::internal::HandleExceptionsInMethodIfSupported<>() 0x174a2a5: testing::Test::Run() 0x174b0f0: testing::TestInfo::Run() ... Google Test internal frames ... [ FAILED ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithFakeSymbolTable/IPv4, where GetParam() = 4-byte object <00-00 00-00> (553 ms) [ RUN ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithRealSymbolTable/IPv4 test/integration/stats_integration_test.cc:377: Failure Expected equality of these values: m_per_cluster Which is: 36954 37115 Stack trace: 0x90e5a9: Envoy::(anonymous namespace)::ClusterMemoryTestRunner_MemoryLargeClusterSizeWithRealSymbolTable_Test::TestBody() 0x174a378: testing::internal::HandleExceptionsInMethodIfSupported<>() 0x174a2a5: testing::Test::Run() 0x174b0f0: testing::TestInfo::Run() ... Google Test internal frames ... [ FAILED ] IpVersions/ClusterMemoryTestRunner.MemoryLargeClusterSizeWithRealSymbolTable/IPv4, where GetParam() = 4-byte object <00-00 00-00> (694 ms)

seems better

source/common/common/utility.cc

source/common/upstream/subset_lb.cc

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

source/common/common/utility.cc

source/common/upstream/subset_lb.cc

source/common/common/utility.cc

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

- attempt to reduce per-cluster memory usage - also fix includes Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

antoniovicente · 2020-07-13T16:08:21Z

Please merge in master and remove draft status when you have a chance.

stale · 2020-07-20T16:31:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sunjayBhatia · 2020-07-21T15:00:40Z

fixing conflicts now

…ered-containers-with-absl Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

antoniovicente · 2020-07-21T23:17:59Z

source/server/overload_manager_impl.cc

-    auto result = actions_.emplace(std::piecewise_construct, std::forward_as_tuple(name),
-                                   std::forward_as_tuple(action, stats_scope));
+    // Cannot use in place construction as OverloadAction constructor may throw
+    auto result = actions_.try_emplace(name, OverloadAction(action, stats_scope));


Are there other places where the emplace and exception interactions come up? Is there an upstream absl issue tracking making the emplace methods exception safe? The status quo seems risky.

Doing a quick grep, the only place I see the piecewise construction happening with emplace is in this file and since we just moved to C++17, we don't seem to have any usage of try_emplace yet

Is this absl issue sufficient? abseil/abseil-cpp#388 we can report another

Reference the issue in the code. How did you detect this exception issue? Should we avoid use of try_emplace until the upstream issue is fixed?

adding a note about that issue and the symptoms

we detected this in a unit test failure:

envoy/test/server/overload_manager_impl_test.cc

Lines 359 to 382 in 142b275

TEST_F(OverloadManagerImplTest, DuplicateTrigger) {

const std::string config = R"EOF(

resource_monitors {

name: "envoy.resource_monitors.fake_resource1"

}

actions {

name: "envoy.overload_actions.dummy_action"

triggers {

name: "envoy.resource_monitors.fake_resource1"

threshold {

value: 0.9

}

}

triggers {

name: "envoy.resource_monitors.fake_resource1"

threshold {

value: 0.8

}

}

}

)EOF";

EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, "Duplicate trigger .*");

}

, when the exception is thrown and memory is freed we got an invalid free

With proper test coverage of any constructor that may throw an exception, I would guess this issue would get caught with a test failure, any in-place construction of a type whose constructor does not throw is safe

I don't don't know if we have full test coverage for every constructor that can throw an exception which is used as a map value. I think the main place where this could come up is the control plane doing config validation in constructors while loading config.

tools/code_format/check_format_test_helper.py

sunjayBhatia · 2020-07-23T14:52:30Z

cc @davinci26 @nigriMSFT

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Existing expectation relied on iteration order of an unordered map, change expectations allow close to be called on separate clients in arbitrary order. Fix also ensures this test works on Windows. Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

…ered-containers-with-absl Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2020-07-24T21:17:19Z

@antoniovicente do you happen to know a hint to fix these clang-tidy errors in tools/clang_tools/api_booster? it seems like the compilation db etc. is already generated for those targets, not sure how else to get it to see gtest.h etc.

antoniovicente · 2020-07-24T21:24:37Z

@antoniovicente do you happen to know a hint to fix these clang-tidy errors in tools/clang_tools/api_booster? it seems like the compilation db etc. is already generated for those targets, not sure how else to get it to see gtest.h etc.

cc @lizan

I don't. The clang errors seem pre-existing and not actionable.

include/envoy/upstream/cluster_manager.h

lizan · 2020-07-25T07:25:15Z

@antoniovicente do you happen to know a hint to fix these clang-tidy errors in tools/clang_tools/api_booster? it seems like the compilation db etc. is already generated for those targets, not sure how else to get it to see gtest.h etc.

cc @lizan

I don't. The clang errors seem pre-existing and not actionable.

In large replace PR like this clang-tidy might pick pre-existing errors, feel free to ignore this.

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

…ered-containers-with-absl Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

include/envoy/upstream/cluster_manager.h

- Replace with absl::node_hash_map/set - Primarily for performance optimizations and to root out any assumptions made about iteration order in tests or otherwise (the replacement absl containers have a non-deterministic iteration order - absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set - Note that a future refactor should reevaluate and move to absl::flat_hash_map/set where possible for memory optimizations - Add format check to disallow future usage of std::unordered_map/set - Small changes made where absl containers required it or tests needed to be modified for correctness Additional Description: - There may be an issue we should open with abseil about `emplace` and `try_emplace` when attempting to do in-place construction - When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees) - //test/server:overload_manager_impl_test demonstrates this - see abseil/abseil-cpp#388 Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang Docs Changes: N/A Release Notes: N/A Fixes envoyproxy#11825 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

- Replace with absl::node_hash_map/set - Primarily for performance optimizations and to root out any assumptions made about iteration order in tests or otherwise (the replacement absl containers have a non-deterministic iteration order - absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set - Note that a future refactor should reevaluate and move to absl::flat_hash_map/set where possible for memory optimizations - Add format check to disallow future usage of std::unordered_map/set - Small changes made where absl containers required it or tests needed to be modified for correctness Additional Description: - There may be an issue we should open with abseil about `emplace` and `try_emplace` when attempting to do in-place construction - When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees) - //test/server:overload_manager_impl_test demonstrates this - see abseil/abseil-cpp#388 Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang Docs Changes: N/A Release Notes: N/A Fixes envoyproxy#11825 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

- Replace with absl::node_hash_map/set - Primarily for performance optimizations and to root out any assumptions made about iteration order in tests or otherwise (the replacement absl containers have a non-deterministic iteration order - absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set - Note that a future refactor should reevaluate and move to absl::flat_hash_map/set where possible for memory optimizations - Add format check to disallow future usage of std::unordered_map/set - Small changes made where absl containers required it or tests needed to be modified for correctness Additional Description: - There may be an issue we should open with abseil about `emplace` and `try_emplace` when attempting to do in-place construction - When a constructor throws an exception, as far as I can tell the c++ language standard says the container should not be affected, however this does not seem to be the case for the absl containers so their guarantees are not the same (though they may be intended to have the same guarantees) - //test/server:overload_manager_impl_test demonstrates this - see abseil/abseil-cpp#388 Risk Level: Low, absl::node_hash_map/set should be drop-in replacements for std::unordered_map/set though this may shake loose more assumptions in tests over time we weren't able to catch locally Testing: Small changes to unit tests, repeatedly run on Windows and Linux/clang Docs Changes: N/A Release Notes: N/A Fixes envoyproxy#11825 Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

'make check' complains about the use of std::unordered_map/set. Replace with abseil equivalents for better performance. Ref. upstream PR envoyproxy/envoy#11879 Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

sunjayBhatia and others added 2 commits July 3, 2020 01:31

sunjayBhatia mentioned this pull request Jul 3, 2020

Replace std::unordered_map with absl::node_hash_map? #11825

Closed

sunjayBhatia added 2 commits July 3, 2020 03:12

Fix failing test, proto repeated field order was a faulty assumption

e272be3

test now passes on Windows Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Simplify test assertion with test utility matcher

a9ac629

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

ggreenway assigned antoniovicente Jul 6, 2020

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

a6ea9bb

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

antoniovicente reviewed Jul 6, 2020

View reviewed changes

sunjayBhatia added 2 commits July 6, 2020 18:08

Fix macos CI and remove extra comment

4124a1d

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

9484ffe

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

antoniovicente reviewed Jul 7, 2020

View reviewed changes

source/common/common/utility.cc Outdated Show resolved Hide resolved

source/common/upstream/subset_lb.cc Show resolved Hide resolved

source/common/common/utility.cc Show resolved Hide resolved

sunjayBhatia added 5 commits July 7, 2020 11:06

pre-existing nit from review feedback

4af5315

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Use flat_hash_set for ClusterSet

d32a826

- attempt to reduce per-cluster memory usage - also fix includes Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

eaaefb2

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Fix format

d40f454

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

add external_dep to overload_manager_interface

f5d5d34

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 20, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 21, 2020

wrowe and others added 2 commits July 21, 2020 11:16

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

313a445

…ered-containers-with-absl Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Fix added unordered_map usage

bd13d7d

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia marked this pull request as ready for review July 21, 2020 16:16

sunjayBhatia requested review from alyssawilk and dio as code owners July 21, 2020 16:16

antoniovicente reviewed Jul 21, 2020

View reviewed changes

sunjayBhatia and others added 6 commits July 23, 2020 12:07

Add more descriptive note and add issue link

880d6e6

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

8b062bd

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Fix redis conn pool test

2d0ae5e

Existing expectation relied on iteration order of an unordered map, change expectations allow close to be called on separate clients in arbitrary order. Fix also ensures this test works on Windows. Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

9127628

…ered-containers-with-absl Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

df6c75c

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Some clang-tidy fixes

47af0d8

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

lizan reviewed Jul 25, 2020

View reviewed changes

include/envoy/upstream/cluster_manager.h Show resolved Hide resolved

sunjayBhatia and others added 6 commits July 27, 2020 09:43

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

00b3935

…ered-containers-with-absl Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

More pre-existing clang-tidy issues

86321fc

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

More clang-tidy

508159e

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Merge remote-tracking branch 'origin/master' into replacing-std-unord…

06a2167

…ered-containers-with-absl Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

Undo clang-tidy change from unrelated find/replace

2df884f

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Undo unrelated find/replace

ab819d2

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

lizan approved these changes Jul 28, 2020

View reviewed changes

include/envoy/upstream/cluster_manager.h Show resolved Hide resolved

lizan merged commit cf2df8c into envoyproxy:master Jul 28, 2020

wrowe deleted the replacing-std-unordered-containers-with-absl branch August 6, 2020 23:04

	TEST_F(OverloadManagerImplTest, DuplicateTrigger) {
	const std::string config = R"EOF(
	resource_monitors {
	name: "envoy.resource_monitors.fake_resource1"
	}
	actions {
	name: "envoy.overload_actions.dummy_action"
	triggers {
	name: "envoy.resource_monitors.fake_resource1"
	threshold {
	value: 0.9
	}
	}
	triggers {
	name: "envoy.resource_monitors.fake_resource1"
	threshold {
	value: 0.8
	}
	}
	}
	)EOF";

	EXPECT_THROW_WITH_REGEX(createOverloadManager(config), EnvoyException, "Duplicate trigger .*");
	}

Conversation

sunjayBhatia commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wrowe commented Jul 6, 2020

Uh oh!

wrowe commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoniovicente commented Jul 6, 2020

Uh oh!

ahedberg commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunjayBhatia Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoniovicente commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Jul 20, 2020

Uh oh!

sunjayBhatia commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sunjayBhatia commented Jul 23, 2020

Uh oh!

sunjayBhatia commented Jul 24, 2020

Uh oh!

antoniovicente commented Jul 24, 2020

Uh oh!

Uh oh!

lizan commented Jul 25, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sunjayBhatia commented Jul 3, 2020 •

edited

Loading

wrowe commented Jul 6, 2020 •

edited

Loading

ahedberg commented Jul 6, 2020 •

edited

Loading

sunjayBhatia Jul 23, 2020 •

edited

Loading

antoniovicente commented Jul 13, 2020 •

edited

Loading