SAMZA-2582: Add a metric to track container failure tracking metric for Samza #1417
Conversation
|
Can you please update the PR description to include what issue/symptom you are fixing with this? It's unclear why you need a container failure metric for each container specifically instead of an aggregate container failure metric. |
|
@cameronlee314 this is needed to be able to track individual container health issues and make informed ops decisions based on that data, this is useful for both containers with host affinity to detect unstable hosts, as well as containers w/o affinity - to detect issues caused by partitioning (i.e. when specific traffic goes to certain containers and causes instability from time to time). |
| @@ -369,6 +369,7 @@ All \<system\>, \<stream\>, \<partition\>, \<store-name\>, \<topic\>, are popula | |||
| | | expired-preferred-host-requests | Number of expired resource-requests-for -preferred-host received by the cluster manager. | | |||
| | | expired-any-host-requests | Number of expired resource-requests-for -any-host received by the cluster manager. | | |||
| | | host-affinity-match-pct | Percentage of non-expired preferred host requests. This measures the % of resource-requests for which host-affinity provided the preferred host. | | |||
| | | \<containerId\>-failure-count | Number of times a container identified by containerId has failed | | |||
There was a problem hiding this comment.
I believe we decided to use "processorId" for 0,1,2..
There was a problem hiding this comment.
that lingo is used internally in code as the naming conventions for javadocs, this is public-facing metrics page where we do not need to have context between processorId and containerId
| @@ -472,6 +479,9 @@ void onResourceCompletedWithUnknownStatus(SamzaResourceStatus resourceStatus, St | |||
| LOG.info("Container ID: {} for Processor ID: {} failed with exit code: {}.", containerId, processorId, exitStatus); | |||
| Instant now = Instant.now(); | |||
| state.failedContainers.incrementAndGet(); | |||
| if (state.perProcessorFailureCount.get(processorId) != null) { | |||
| state.perProcessorFailureCount.get(processorId).incrementAndGet(); | |||
| } | |||
There was a problem hiding this comment.
else {
Log.error("Unknown/orphan container") ??
}
There was a problem hiding this comment.
This method is the helper to and is invoked from onResourceCompleted(...) which does the check for processorId to be legit, remeber that we also get redundant notifications so we cannot declare a container orphan / unknown, we need more testing to deem callback senarios as orphans and that work is beyond the scope of this change
|
|
||
| /** | ||
| * Map of the Samza processor ID to the count of failed attempts | ||
| * Modified by AMRMCallbackThread | ||
| */ | ||
| public final ConcurrentMap<String, AtomicInteger> perProcessorFailureCount = new ConcurrentHashMap<>(0); | ||
|
|
There was a problem hiding this comment.
If this information is only useful for metric-emission, does it need to be stored in "state" ?
There was a problem hiding this comment.
That is correct, we can directly wire metrics registry in ContainerManager, ContainerAllocator and instantiate new guage and counters in the code but all metrics related to AM are under this ContainerProcessManagerMetrics class which holds MetricsRegistry and SamzaApplicationState, so once does not need to wire MetricsRegistry individually to each AM class ContainerManager, ContainerAllocator. This is the justification for maintained this state variable to wire metrics, I feel this approach is cleaner
There was a problem hiding this comment.
Check SamzaApplicationState most of the state there is just used for metric emissions
| @@ -236,6 +237,12 @@ public void start() { | |||
| Map<String, String> processorToHostMapping = state.jobModelManager.jobModel().getAllContainerLocality(); | |||
| containerAllocator.requestResources(processorToHostMapping); | |||
|
|
|||
| // Initialize the per processor failure count to be 0 | |||
| processorToHostMapping.keySet().forEach(processorId -> { | |||
There was a problem hiding this comment.
See comment below on how/why this information isnt really in "state"
Changes: Added a metric to -failure-count to track failure count of a single container
API Changes: None
Tests: Tested the change with a yarn job deploy
Upgrade Instructions: None
Usage Instructions: None