Implement cool off handling for the Publisher #272
Conversation
The Publisher shouldn't spam the same action over and over again. It should wait for a specified period of time before repeating an action. This commit implements this logic.
Codecov Report
@@ Coverage Diff @@
## master #272 +/- ##
============================================
+ Coverage 66.33% 66.35% +0.02%
- Complexity 1795 1799 +4
============================================
Files 270 270
Lines 11990 12002 +12
Branches 952 954 +2
============================================
+ Hits 7953 7964 +11
+ Misses 3722 3721 -1
- Partials 315 317 +2
Continue to review full report at Codecov.
|
...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyQueueCapacityAction.java
Outdated
Show resolved
Hide resolved
...om/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/deciders/Publisher.java
Show resolved
Hide resolved
...mazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/deciders/PublisherTest.java
Outdated
Show resolved
Hide resolved
...om/amazon/opendistro/elasticsearch/performanceanalyzer/decisionmaker/deciders/Publisher.java
Outdated
Show resolved
Hide resolved
| @@ -30,7 +30,7 @@ | |||
| public class ModifyQueueCapacityAction implements Action { | |||
|
|
|||
| public static final String NAME = "modify_queue_capacity"; | |||
| public static final int COOL_OFF_PERIOD_IN_SECONDS = 300; | |||
| public static final long COOL_OFF_PERIOD_IN_MILLIS = 300 * 1_000; | |||
There was a problem hiding this comment.
can we set this in rca.conf and make this configurable
There was a problem hiding this comment.
Good idea, but out of scope for this commit. The only reason I changed this line is that we're going from s units to ms
| } | ||
|
|
||
| return new EmptyFlowUnit(System.currentTimeMillis()); | ||
| return new EmptyFlowUnit(Instant.now().toEpochMilli()); |
There was a problem hiding this comment.
any specific reason to replace system with Instant.now() ?
There was a problem hiding this comment.
|
|
||
| private Collator collator; | ||
| private boolean isMuted = false; | ||
| private Map<String, Long> actionToExecutionTime; |
There was a problem hiding this comment.
Can we use Action itself as the key ?
There was a problem hiding this comment.
We could, but we'd have to implement hashCode() and equals() for all Actions. I asked Vigya, and he said it was a safe assumption that Action names are unique, so it should suffice as a key.
| if (elapsed >= action.coolOffPeriodInMillis()) { | ||
| return true; | ||
| } else { | ||
| LOG.debug("Action {} still has {} ms left in its cool off period", action.name(), |
There was a problem hiding this comment.
Let's keep the logging format consistent within RCA package. Let's add the "Publisher: " at the front of this log
There was a problem hiding this comment.
I'm against this because it's an anti-pattern in the log4j context. If we want to include class info in logging lines we should update our logging pattern to include %c{1.} for example. See https://logging.apache.org/log4j/2.x/manual/layouts.html
Does anyone else have thoughts on this?
There was a problem hiding this comment.
I added this in for now
| LOG.info("Executing action: [{}]", action.name()); | ||
| action.execute(); | ||
| if (isCooledOff(action)) { // Only execute actions which have passed their cool off period | ||
| LOG.info("Executing action: [{}]", action.name()); |
There was a problem hiding this comment.
also same as above, I'd like to hear people's thoughts on this
| @@ -18,33 +18,65 @@ | |||
| import com.amazon.opendistro.elasticsearch.performanceanalyzer.decisionmaker.actions.Action; | |||
| import com.amazon.opendistro.elasticsearch.performanceanalyzer.rca.framework.core.NonLeafNode; | |||
| import com.amazon.opendistro.elasticsearch.performanceanalyzer.rca.scheduler.FlowUnitOperationArgWrapper; | |||
|
|
|||
Issue #, if available: N/A
Description of changes:
The Publisher shouldn't spam the same action over and over again. It
should wait for a specified period of time before repeating an action.
This commit implements this logic.
Tests: PublisherTest
Code coverage percentage for this patch: See CodeCov Report
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.