wilfred-s commented on code in PR #146:
URL: https://github.com/apache/yunikorn-web/pull/146#discussion_r1423359293
##########
src/app/services/scheduler/scheduler.service.ts:
##########
@@ -328,13 +328,13 @@ export class SchedulerService {
const formatted = [];
if (resource && resource.memory !== undefined) {
- formatted.push(`Memory: ${CommonUtil.formatBytes(resource.memory)}`);
+ formatted.push(`Memory:
${CommonUtil.formatMemoryBytes(resource.memory)}`);
} else {
formatted.push(`Memory: ${NOT_AVAILABLE}`);
}
if (resource && resource.vcore !== undefined) {
- formatted.push(`CPU: ${CommonUtil.formatCount(resource.vcore)}`);
+ formatted.push(`CPU: ${CommonUtil.formatCpuCore(resource.vcore)}`);
Review Comment:
> When making this change, I noticed that if the resource is undefined, the
result will be empty. Even though this might not happen in the real world, I
prefer to keep the original behavior.
I think showing that we have no keys at all in the end is better than
showing `memory` and `cpu` always. Showing `memory` and `cpu` is a choice
pointing back to a time that `memory` and `cpu` were required types. They have
never been that in YuniKorn. If a resource is completely empty we should show
that as `n/a` I think. Even shortcut that all the way in the beginning of the
conversion code.
This empty resource by the way only affects queues, nodes and applications
when it comes to certain resource values. Even for the best effort pods there
is no `memory` or `CPU` but we always set the `pods` resource.
Having a fixed order is good, I like that side of the change. `memory` and
`CPU` should be first in the list if set. Extending that with
`ephemeral-storage` is good. I would add `pods` in that list too (even before
storage)
--
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]
