-
Notifications
You must be signed in to change notification settings - Fork 20
Implement Action Flip Flop Detection in the Publisher #287
Implement Action Flip Flop Detection in the Publisher #287
Conversation
Codecov Report
@@ Coverage Diff @@
## master #287 +/- ##
============================================
+ Coverage 67.26% 67.31% +0.05%
- Complexity 1949 1974 +25
============================================
Files 286 288 +2
Lines 12712 12766 +54
Branches 1034 1040 +6
============================================
+ Hits 8551 8594 +43
- Misses 3798 3809 +11
Partials 363 363
Continue to review full report at Codecov.
|
ae89583
to
df5ad95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, few renaming related comments but good to merge.
...pendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/TimedFlipFlopDetector.java
Outdated
Show resolved
Hide resolved
...pendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/TimedFlipFlopDetector.java
Outdated
Show resolved
Hide resolved
...pendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/TimedFlipFlopDetector.java
Outdated
Show resolved
Hide resolved
...om/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/deciders/Publisher.java
Outdated
Show resolved
Hide resolved
...pendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/TimedFlipFlopDetector.java
Show resolved
Hide resolved
...pendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/TimedFlipFlopDetector.java
Show resolved
Hide resolved
e9f53d1
to
290f6ae
Compare
...ava/com/amazon/opendistro/elasticsearch/performanceanalyzer/collections/TimeExpiringSet.java
Show resolved
Hide resolved
...ava/com/amazon/opendistro/elasticsearch/performanceanalyzer/collections/TimeExpiringSet.java
Outdated
Show resolved
Hide resolved
...ava/com/amazon/opendistro/elasticsearch/performanceanalyzer/collections/TimeExpiringSet.java
Outdated
Show resolved
Hide resolved
* @return true if the impact for any given Dimension in curr is a flip flop Impact when compared to | ||
* the impact for a given dimension in prev | ||
*/ | ||
protected boolean isFlipFlopVector(ImpactVector prev, ImpactVector curr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we will need the resource name for which the flip flop happened ?
So, for for prev = (HEAP: INCREASE, CPU: INCREASE), curr = (HEAP: DECREASE, CPU: INCREASE)
, we can return HEAP
instead of Boolean value ?
What I want to understand is if we use(or might use in future) this information anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's trivial to add this information in the future by extracting the logic in this function, I think we can table this until the need arises
...pendistro/elasticsearch/performanceanalyzer/decisionmaker/actions/TimedFlipFlopDetector.java
Show resolved
Hide resolved
...om/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/deciders/Publisher.java
Show resolved
Hide resolved
We don't want to rubber-band when applying Actions (e.g. applying a CPU increase right after we apply a CPU decrease). This commit implements logic which allows the Publisher to reject Actions which flip flop.
290f6ae
to
f34ed6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Issue #: #286
Description of changes:
We don't want to rubber-band when applying Actions (e.g. applying a CPU
increase right after we apply a CPU decrease).
This commit implements logic which allows the Publisher to reject
Actions which flip flop.
Tests:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.