pan3793 commented on PR #8674:
URL: https://github.com/apache/hadoop/pull/8674#issuecomment-5340132586
Scanned with an LLM and posted the selected feedback below
DISCLAIMER: I review each finding before posting, they look reasonable to me
literally, but I'm not an expert in the frontend area, please re-evaluate each
item carefully (you can reject the item with reasons if you think it's wrong).
Findings
1. Auto-created parents show the wrong template.
getAutoCreatedQueueTemplatePath (templateUtils.ts:78) always picks
FLEXIBLE_LEAF for every dynamicFlexible queue. But flexible auto-creation makes
parents dynamic too -
CapacitySchedulerInfoHelper documents "both a parent and a leaf can be
either static or dynamicFlexible", and parent capacity comes from
auto-queue-creation-v2.parent-template
(AutoCreatedQueueTemplate.parentOnlyProperties). So for an app
targeting root.default.team.user, the auto-created root.default.team card
shows the leaf weight (3w) instead of the parent weight. Fix: branch on
queueInfo.queueType and use FLEXIBLE_PARENT for parents.
2. Template overrides the queue's own staged value.
resolveAutoCreatedQueueCapacityConfigs (templateUtils.ts:122-129)
unconditionally replaces the queue value with the template value. The card's
"Edit Capacity" is enabled for auto-created
queues (QueueCardContextMenu.tsx:126) and staging a change writes capacity
on the queue path (capacityEditorSlice.ts:431); getQueuePropertyValue is
staged-aware (queueDataSlice.ts:37-51). After editing 3w -> 5w, the card still
shows 3w. Pre-
trunk behavior showed the staged value, so this is a regression of the
change. Fix: use the template only when the queue's own value is empty/unset,
and never when the own value is staged.
3. Nested/wildcard templates are missed. The helper reconstructs
<parent>.auto-queue-creation-v2.leaf-template and looks it up in the config
map. For depth >= 2 auto-created queues, the effective leaf-template lives at
root.default.*.auto-
queue-creation-v2.leaf-template (wildcard inheritance,
AutoCreatedQueueTemplate.setTemplateConfigEntries), so the lookup returns empty
and the 0% display persists. The RM already resolves per-queue template
properties and ships them in the
scheduler REST as autoQueueLeafTemplateProperties /
autoQueueParentTemplateProperties (present in the UI's CapacitySchedulerInfo
type, types/scheduler.ts:90-92, though not yet on QueueInfo). Reading those
fields per queue would fix findings 1
and 3 at once and drop the path-guessing entirely.
4. Nit: formatting. templateUtils.ts:87 is 105 chars, over the repo
printWidth: 100; prettier --check (format:check) would flag it. The
two-blank-line import block also reads oddly.
5. Nit: test coverage. Tests are good and idiomatic, but they don't cover
the parent-template case, staged-edit precedence, or nested/wildcard templates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
