Handling empty flow unit during Cache/Queue RCA execution #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sruti1312
reviewed
Jul 30, 2021
| // If the RCA receives 3 empty flow units, re-set the 'hasMetric' value | ||
| hasEvictions = false; | ||
| clearCounter = 0; | ||
| LOG.error( |
There was a problem hiding this comment.
For some reason if there are only empty flowunits, this might flood the logs. As we are emitting a metric, can we modify the log level to debug?
There was a problem hiding this comment.
Good point, let me change the log level to debug.
| @@ -217,17 +218,21 @@ public void generateFlowUnitListFromWire(FlowUnitOperationArgWrapper args) { | |||
| private static class CacheCollector { | |||
| private final Resource cache; | |||
| private final Metric cacheMetrics; | |||
| private boolean hasMetric; | |||
| private boolean hasCacheMetric; | |||
There was a problem hiding this comment.
Can we name this variable with the metric name that it denotes? Like the previous one, hasEvictions?
There was a problem hiding this comment.
the metric type for ShardRequestCache can be evictions and hit both. So, kept it generic.
| // If the RCA receives 3 empty flow units, re-set the 'hasCacheMetric' value | ||
| hasCacheMetric = false; | ||
| clearCounter = 0; | ||
| LOG.error( |
There was a problem hiding this comment.
Same as above, can we make it to debug?
There was a problem hiding this comment.
Agreed. Moving it to debug.
sruti1312
approved these changes
Jul 30, 2021
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
**Is your feature request related to a problem? Please provide an existing Issue #33
[RCA] Handle Empty FU within Cache and Queue RCAs
In this case, PA was unable to persist value for 64 metric for the one hour window and thus, RCA was running into Cache_Request_Hit metric table does not exist. Returning null for the metric/dimension and QueryMetric was returning empty Flow units to ShardRequestCacheRCA. The ShardRequestCacheRCA on finding an empty FU, ignores it. This was done to accommodate any delays in metric persistence from PA.
Before this event, the ShardReqestCacheRCA had hasMetric set to true and this variable was not reset during the 1 hour (RCA was ignoring the empty flow units). Thus, the RCA was in Unhealthy state and CacheDecider was repeatedly suggesting ‘ModifyCacheMaxSize’.
This same code is shared across Queue RCA and [FieldDataCacheRca](https://github.com/opensearch-project/performance-analyzer-rca/blob/main/src/main/java/org/opensearch/performanceanalyzer/rca/store/rca/cache/FieldDataCacheRca.java, so we will need to fix this there as well
Describe the solution you are proposing
A clear and concise description of what you want to happen.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.