Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Outlier detection config #543

Merged
merged 7 commits into from
Mar 9, 2017
Merged

Outlier detection config #543

merged 7 commits into from
Mar 9, 2017

Conversation

rshriram
Copy link
Member

@rshriram rshriram commented Mar 7, 2017

fixes #540

@mattklein123
Copy link
Member

needs docs

ccaraman
ccaraman reviewed Mar 7, 2017
View reviewed changes
source/common/upstream/outlier_detection_impl.cc Outdated
EventLoggerPtr event_logger)
: dispatcher_(dispatcher), runtime_(runtime), time_source_(time_source),
DetectorConfig::DetectorConfig(const Json::ObjectPtr& json_config) {
interval_ms_ = static_cast<uint64_t>(json_config->getInteger("interval_ms", 10000));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move these to the initializer list

mattklein123
mattklein123 reviewed Mar 7, 2017
View reviewed changes
source/common/upstream/outlier_detection_impl.cc Outdated
@@ -17,8 +17,8 @@ DetectorPtr DetectorImplFactory::createForCluster(Cluster& cluster,
// Right now we don't support any configuration but in order to make the config backwards
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete comment

source/common/upstream/outlier_detection_impl.cc Outdated
@@ -17,8 +17,8 @@ DetectorPtr DetectorImplFactory::createForCluster(Cluster& cluster,
// Right now we don't support any configuration but in order to make the config backwards
// compatible we just look for an empty object.
if (cluster_config.hasObject("outlier_detection")) {
return DetectorImpl::create(cluster, dispatcher, runtime, ProdSystemTimeSource::instance_,
event_logger);
return DetectorImpl::create(cluster, cluster_config.getObject("outlier_detection", true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't "allow_empty" here (remove true param), the object must exist in this branch.

test/common/upstream/outlier_detection_impl_test.cc Outdated
@@ -43,6 +43,27 @@ TEST(OutlierDetectorImplFactoryTest, Detector) {
DetectorImplFactory::createForCluster(cluster, *loader, dispatcher, runtime, nullptr));
}

TEST(OutlierDetectorImplFactoryTest, DetectorConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really a useful test. Can you test the config object directly, and make sure the loaded values are what you expect (or do that additionally)

@rshriram rshriram force-pushed the outlier_detection_config branch from 45fb071 to f15d8ae Compare March 7, 2017 23:45
ccaraman
ccaraman reviewed Mar 7, 2017
View reviewed changes
source/common/upstream/outlier_detection_impl.h Outdated
uint64_t enforcing() { return enforcing_; }

private:
const uint64_t interval_ms_{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{} is no longer needed since you are using the initializer list.

@junr03 junr03 mentioned this pull request Mar 8, 2017
Merged
5 tasks
mattklein123
mattklein123 reviewed Mar 9, 2017
View reviewed changes
source/common/upstream/outlier_detection_impl.h Outdated
*/
class DetectorConfig {
public:
DetectorConfig(const Json::ObjectPtr& json_config);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pass const Json::Object&

source/common/upstream/outlier_detection_impl.h Outdated
SystemTimeSource& time_source,
EventLoggerPtr event_logger);
static std::shared_ptr<DetectorImpl>
create(const Cluster& cluster, const Json::ObjectPtr& json_config, Event::Dispatcher& dispatcher,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pass const Json::Object&

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. fixed everywhere.

source/common/upstream/outlier_detection_impl.h Outdated

// Upstream::Outlier::Detector
void addChangedStateCb(ChangeStateCb cb) override { callbacks_.push_back(cb); }

private:
DetectorImpl(const Cluster& cluster, Event::Dispatcher& dispatcher, Runtime::Loader& runtime,
DetectorImpl(const Cluster& cluster, const Json::ObjectPtr& json_config,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pass const Json::Object&

source/common/upstream/outlier_detection_impl.cc Outdated
if (cluster_config.hasObject("outlier_detection")) {
return DetectorImpl::create(cluster, dispatcher, runtime, ProdSystemTimeSource::instance_,
event_logger);
return DetectorImpl::create(cluster, cluster_config.getObject("outlier_detection", false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you don't need false param, it's the default.

@rshriram rshriram force-pushed the outlier_detection_config branch from 619f65e to 97209f3 Compare March 9, 2017 19:26
@mattklein123 mattklein123 merged commit e49093e into envoyproxy:master Mar 9, 2017
@rshriram rshriram deleted the outlier_detection_config branch March 23, 2017 15:29
jplevyak pushed a commit to istio/envoy that referenced this pull request Jul 9, 2020
@lizan
make WasmService as bootstrap extension (envoyproxy#543)
cc7b50f 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
@junr03 @jpsim
core: integration with Envoy's API listener (#616)
fe8de67 
Description: this PR integrates [Envoy's ApiListener](#9516) to replace Envoy Mobile's use of the AsyncClient. Doing so gives Envoy Mobile access to all L7 facilities provided by Envoy's Http Connection Manager; including but no limited to the L7 filters.
Risk Level: HIGH! This PR changes the underlying code for Envoy Mobile's core functionality
Testing: unit tests updated for the dispatcher, integration tests, and e2e tests with the example apps. Additionally, the ApiListener code has been unit and integration tested in Envoy.

Fixes #543

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
@junr03 @jpsim
core: integration with Envoy's API listener (#616)
cec88b7 
Description: this PR integrates [Envoy's ApiListener](#9516) to replace Envoy Mobile's use of the AsyncClient. Doing so gives Envoy Mobile access to all L7 facilities provided by Envoy's Http Connection Manager; including but no limited to the L7 filters.
Risk Level: HIGH! This PR changes the underlying code for Envoy Mobile's core functionality
Testing: unit tests updated for the dispatcher, integration tests, and e2e tests with the example apps. Additionally, the ApiListener code has been unit and integration tested in Envoy.

Fixes #543

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccaraman ccaraman ccaraman left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support config file params for outlier detection
3 participants
@rshriram @mattklein123 @ccaraman