Repository navigation
YARN-11995. Preserve custom resource allocations in timeline container history - #8770
Conversation
…r history Generated-by: OpenAI Codex
|
💔 -1 overall
This message was automatically generated. |
|
CI update for Jenkins run #1: The fork Build passes, and the changed
Neither failing path is changed by this PR. Once those fixes land, a fresh CI run should establish whether any failures remain. |
@gerashegalov Thanks for inviting me to review this PR! I'll take another look. I suggest merging #8776 and #8779 first, as they address unrelated flaky YARN tests that can cause CI failures. This should help stabilize the CI baseline and make it easier to assess this PR's results. Thanks for your patience! |
|
🎊 +1 overall
This message was automatically generated. |
|
@gerashegalov The latest Jenkins/Yetus run is green. The two remaining red checks appear to be stale statuses from the previous build. Could you push an empty commit to retrigger CI on a new commit SHA? This should prevent the stale statuses from carrying over. |
joseluisll
left a comment
joseluisll
left a comment
There was a problem hiding this comment.
Thanks for the thorough patch and tests. Routing every reader through TimelineUtils.getContainerResource is a nice cleanup, and moving off the (Integer) casts also fixes a latent ClassCastException once memory comes back from JSON as a Long. I've left a few inline suggestions in TimelineUtils.
| LOG.warn("Skipping unknown resource type {} in container history", name); | ||
| continue; | ||
| } | ||
| Map<?, ?> allocation = (Map<?, ?>) entry.getValue(); |
There was a problem hiding this comment.
Make per-entry parsing tolerant of bad data
This loop has no error handling, so one malformed entry fails the whole conversion:
- a missing
valuegives an NPE; - a value or units of an unexpected type gives a
ClassCastException; - an unknown unit, or a unit conversion that overflows, gives an
IllegalArgumentExceptionfromUnitsConversionUtil.convert.
Unknown resource types already get "warn and skip". I think bad entries deserve the same treatment instead of failing the request. Otherwise one bad entry breaks the container report, and on the list endpoints it breaks every container of the attempt. Something like:
try {
Map<?, ?> allocation = (Map<?, ?>) entry.getValue();
long value = ((Number) allocation.get("value")).longValue();
String units = (String) allocation.get("units");
String defaultUnits = resource.getResourceInformation(name).getUnits();
resource.setResourceValue(name,
UnitsConversionUtil.convert(units, defaultUnits, value));
} catch (RuntimeException e) {
LOG.debug("Skipping malformed allocation for resource type {}", name, e);
}A small test with a missing value and with an unknown unit would cover it.
There was a problem hiding this comment.
Thanks for the concrete cases. Addressed in 48b26fe: malformed custom entries are skipped independently so later valid allocations survive. The tests cover missing values, invalid shapes, unknown units, conversion overflow, and invalid numeric values. The separate pre-existing UnitsConversionUtil issue is tracked as YARN-12007.
| continue; | ||
| } | ||
| if (!ResourceUtils.getResourceTypes().containsKey(name)) { | ||
| LOG.warn("Skipping unknown resource type {} in container history", name); |
There was a problem hiding this comment.
Avoid log spam for unknown resource types
This warning runs for every container on every read. For example, take an AHS without resource-types.xml that serves a container list for a large app. It would write one warning per container per request. Could we log this at DEBUG, or warn only once per resource type (e.g. track the names already reported in a ConcurrentHashMap.newKeySet())?
There was a problem hiding this comment.
Addressed in 48b26fe: unknown resource types now log at DEBUG rather than WARN, and both timeline docs match.
| } | ||
|
|
||
| @Private | ||
| public static Map<String, Map<String, Object>> getCustomResourceInfo( |
There was a problem hiding this comment.
Skip the field when there are no custom resources
This always returns a map, and the publishers always put it. So on clusters without custom resources, every container entity gets an empty YARN_CONTAINER_ALLOCATED_RESOURCES: {}. That's an extra column per container in ATSv2/HBase and extra bytes in ATSv1. Readers already handle the field being absent, so the publishers could skip the put when the map is empty.
There was a problem hiding this comment.
Addressed in 48b26fe: all three publishers omit the field for an empty custom-resource map. The helper also omits zero-valued custom types, so even a configured but unused GPU type adds no field. RM v1, RM v2, and NM tests assert the field is absent.
Generated-by: OpenAI Codex Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
|
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
|
🎊 +1 overall
This message was automatically generated. |
joseluisll
left a comment
joseluisll
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. 48b26fe addresses all three earlier points. A bad entry now skips only itself, unknown types log at DEBUG, and the publishers leave out the field when there's nothing to store. The tests cover the edge cases well. I've left a few small suggestions inline. None of them blocks this. +1 (non-binding).
| && !ResourceInformation.VCORES_URI.equals(name) | ||
| && information.getValue() != 0) { | ||
| Map<String, Object> allocation = new HashMap<>(); | ||
| allocation.put("value", information.getValue()); |
There was a problem hiding this comment.
nit, which I should have caught on the first pass: "value" and "units" are spelled out separately by the writer (getCustomResourceInfo) and the reader (getContainerResource). They're part of the stored format now, so could they be constants next to ALLOCATED_RESOURCES_INFO in ContainerMetricsConstants? That way writer and reader can't drift apart.
There was a problem hiding this comment.
Thanks—addressed in d60acf7. value and units now have shared constants in ContainerMetricsConstants, used by both the writer and reader. The stored field names are unchanged.
| } | ||
|
|
||
| @Private | ||
| public static Map<String, Map<String, Object>> getCustomResourceInfo( |
There was a problem hiding this comment.
nit, also from the first pass: A short Javadoc on these two public helpers would help. On getCustomResourceInfo it could show the map shape (name -> {value, units}, leaving out memory, vcores and zero values). On getContainerResource it could say that unknown or malformed entries are skipped.
There was a problem hiding this comment.
Added Javadocs in d60acf7 describing the stored map shape, omitted allocations, and how the reader handles unknown or malformed entries.
| String defaultUnits = resource.getResourceInformation(name).getUnits(); | ||
| resource.setResourceValue(name, | ||
| UnitsConversionUtil.convert(units, defaultUnits, value)); | ||
| } catch (ClassCastException | NullPointerException |
There was a problem hiding this comment.
optional: Following up on my earlier suggestion: listing the four exceptions covers today's code paths. A single catch (RuntimeException e) would also cover anything not listed, such as a ResourceNotFoundException from getResourceInformation, which is what a per-entry skip guard is for. Feel free to leave as is.
There was a problem hiding this comment.
Thanks for the optional suggestion. I’d keep the specific catches here: after the configured-type check, a ResourceNotFoundException would indicate inconsistent resource metadata rather than a malformed stored entry, so silently skipping it could hide a separate problem.
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
|
🎊 +1 overall
This message was automatically generated. |
slfan1989
left a comment
slfan1989
left a comment
There was a problem hiding this comment.
@gerashegalov Thanks for the contribution! LGTM.
The patch covers all three timeline publishers and both history report converters while preserving compatibility with existing memory/vcore fields and legacy entities. Malformed custom allocations are skipped individually, and the new field is omitted when no nonzero custom allocation exists.
I checked out the current PR head and ran the seven related test classes on Java 17: all 112 tests passed, with zero failures, errors, or skipped tests. Build and Yetus are also green.
The resource-type configuration requirement and limitations for older history data are clearly documented.
From my side, this PR is ready to merge whenever you’re ready.
|
Thank you for reviewing @joseluisll @slfan1989 @slfan1989 would you mind merging the PR if there are no more comments? |
|
Merged. Thanks, @gerashegalov, for the contribution, and @joseluisll for reviewing! @gerashegalov, should we also backport this change to |
|
Thank you @slfan1989 . I think it would be great to backport to branch-3.4, which hopefully speeds up its delivery to customers if the effort is reasonable. I can work on it next |
4 participants
Description of PR
Fixes YARN-11995.
Container history currently drops custom allocations such as
yarn.io/gpu: the timeline publishers store only memory and vcores, and the history readers reconstruct a two-resource report. An allocation of two GPUs therefore appears as zero in history.Persist nonzero custom allocation values and units in the additive
YARN_CONTAINER_ALLOCATED_RESOURCESinfo field. Update both ResourceManager timeline publishers, the NodeManager v2 publisher, and both history report converters. The publishers omit the field when no nonzero custom allocation exists, including on clusters with configured but unused resource types.Shared helpers snapshot allocations before asynchronous publication and convert stored units to the reader's configured units. Readers skip unknown types at DEBUG and skip malformed allocations individually, preserving later valid entries. Integral values are checked for range and precision; floating-point values are skipped because JSON decoding may already have rounded them.
Existing memory/vcore fields and legacy entities remain compatible. History servers and standalone clients need the same resource-type configuration to include custom allocations in reports. Allocations omitted by older publishers cannot be recovered retroactively. The pre-existing shared unit-conversion overflow is tracked separately in YARN-12007.
How was this patch tested?
TestTimelineServiceHelper,TestSystemMetricsPublisher,TestSystemMetricsPublisherForV2,TestNMTimelinePublisher,TestApplicationHistoryManagerOnTimelineStore,TestAHSWebServices, andTestAHSv2ClientImpl.-Dmaven.test.failure.ignore=falseand zero failures, errors, or skips.resource-types.xml.git diff --checkpassed. The original patch's checkstyle run reported 45 existing diagnostics in the checked files, none on changed lines; checkstyle has not been rerun locally for the feedback update.For code changes:
declared according to the connector-specific documentation? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required. Not applicable: no object storage changes.
LICENSE,LICENSE-binary,NOTICE-binaryfiles? Not applicable: no licensing changes.AI Tooling
Contains content generated by OpenAI Codex.
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html