wilfred-s commented on code in PR #146:
URL: https://github.com/apache/yunikorn-web/pull/146#discussion_r1423361111
##########
src/app/services/scheduler/scheduler.service.ts:
##########
@@ -325,48 +325,66 @@ export class SchedulerService {
}
private formatResource(resource: SchedulerResourceInfo): string {
- const formatted = [];
-
- if (resource && resource.memory !== undefined) {
- formatted.push(`Memory: ${CommonUtil.formatBytes(resource.memory)}`);
- } else {
- formatted.push(`Memory: ${NOT_AVAILABLE}`);
- }
-
- if (resource && resource.vcore !== undefined) {
- formatted.push(`CPU: ${CommonUtil.formatCount(resource.vcore)}`);
- } else {
- formatted.push(`CPU: ${NOT_AVAILABLE}`);
- }
+ const formatted: string[] = [];
+ if (resource) {
+ // Object.keys() didn't guarantee the order of keys, sort it before
iterate.
+ Object.keys(resource).sort(this.resourcesCompareFn).forEach((key) => {
+ let value = resource[key];
+ let formattedKey = key;
+ let formattedValue = NOT_AVAILABLE;
- if (resource){
- Object.keys(resource).forEach((key) => {
switch(key){
case "memory":
- case "vcore":{
+ formattedKey = "Memory";
+ formattedValue = CommonUtil.formatMemoryBytes(value);
+ break;
+ case "vcore":
+ formattedKey = "CPU";
+ formattedValue = CommonUtil.formatCpuCore(value);
break;
- }
- case "ephemeral-storage":{
- if (resource[`ephemeral-storage`] == 0) {
- formatted.push(`ephemeral-storage: ${NOT_AVAILABLE}`);
- }else{
- formatted.push(`ephemeral-storage:
${CommonUtil.formatBytes(resource[key])}`);
+ case "ephemeral-storage":
+ if (value !== 0){ // if value is 0, show NOT_AVAILABLE
+ formattedValue = CommonUtil.formatEphemeralStorageBytes(value);
}
break;
- }
- default:{
- if (resource[key] == 0) {
- formatted.push(`${key}: ${NOT_AVAILABLE}`);
- }else{
- formatted.push(`${key}:
${CommonUtil.formatOtherResource(resource[key])}`);
+ default:
+ if (value !== 0){ // if value is 0, show NOT_AVAILABLE
+ if (key.startsWith('hugepages-')) {
+ formattedValue = CommonUtil.formatMemoryBytes(value);
+ } else{
+ formattedValue = CommonUtil.formatOtherResource(value);
+ }
}
break;
- }
- }
+ }
+ // }
+ formatted.push(`${formattedKey}: ${formattedValue}`);
});
}
-
+
+ // Set CPU/Memory as NOT_AVAILABLE if it was not in Object.keys() or
resource is undefined.
+ if (!resource || resource.vcore === undefined)
+ formatted.unshift(`CPU: ${NOT_AVAILABLE}`);
+ if (!resource || resource.memory === undefined)
+ formatted.unshift(`Memory: ${NOT_AVAILABLE}`);
return formatted.join(', ');
+ }
+
+ private resourcesCompareFn(a: string, b: string): number {
+ // define the order of resources
+ const resourceOrder: { [key: string]: number } = {
+ "memory": 1,
+ "vcore": 2,
+ "ephemeral-storage": 3
Review Comment:
Should add at least pods, before ephemeral-storage
##########
src/app/utils/common.util.ts:
##########
@@ -30,34 +30,55 @@ export class CommonUtil {
return uniqid;
}
- static formatBytes(value: number | string): string {
- const units: readonly string[] = ['kB', 'MB', 'GB', 'TB', 'PB'];
+ static formatMemoryBytes(value: number | string): string {
+ const units: readonly string[] = ['KiB', 'MiB', 'GiB', 'TiB', 'PiB',
'EiB'];
let unit: string = 'bytes';
Review Comment:
This should be a `B` for consistency with other units.
##########
src/app/services/scheduler/scheduler.service.ts:
##########
@@ -325,48 +325,66 @@ export class SchedulerService {
}
private formatResource(resource: SchedulerResourceInfo): string {
- const formatted = [];
-
- if (resource && resource.memory !== undefined) {
- formatted.push(`Memory: ${CommonUtil.formatBytes(resource.memory)}`);
- } else {
- formatted.push(`Memory: ${NOT_AVAILABLE}`);
- }
-
- if (resource && resource.vcore !== undefined) {
- formatted.push(`CPU: ${CommonUtil.formatCount(resource.vcore)}`);
- } else {
- formatted.push(`CPU: ${NOT_AVAILABLE}`);
- }
+ const formatted: string[] = [];
+ if (resource) {
+ // Object.keys() didn't guarantee the order of keys, sort it before
iterate.
+ Object.keys(resource).sort(this.resourcesCompareFn).forEach((key) => {
+ let value = resource[key];
+ let formattedKey = key;
+ let formattedValue = NOT_AVAILABLE;
- if (resource){
- Object.keys(resource).forEach((key) => {
switch(key){
case "memory":
- case "vcore":{
+ formattedKey = "Memory";
+ formattedValue = CommonUtil.formatMemoryBytes(value);
+ break;
+ case "vcore":
+ formattedKey = "CPU";
+ formattedValue = CommonUtil.formatCpuCore(value);
break;
- }
- case "ephemeral-storage":{
- if (resource[`ephemeral-storage`] == 0) {
- formatted.push(`ephemeral-storage: ${NOT_AVAILABLE}`);
- }else{
- formatted.push(`ephemeral-storage:
${CommonUtil.formatBytes(resource[key])}`);
+ case "ephemeral-storage":
+ if (value !== 0){ // if value is 0, show NOT_AVAILABLE
Review Comment:
we need to be consistent: either show `0` for all or `n/a` for all. Now we
have some showing `n/a` and memory / cpu showing `0 bytes` or `0`
##########
src/app/utils/common.util.ts:
##########
@@ -30,34 +30,55 @@ export class CommonUtil {
return uniqid;
}
- static formatBytes(value: number | string): string {
- const units: readonly string[] = ['kB', 'MB', 'GB', 'TB', 'PB'];
+ static formatMemoryBytes(value: number | string): string {
+ const units: readonly string[] = ['KiB', 'MiB', 'GiB', 'TiB', 'PiB',
'EiB'];
let unit: string = 'bytes';
- let toValue = +value
+ let toValue = +value;
+ for (let i = 0, unitslen = units.length; toValue / 1024 >= 1 && i <
unitslen;i = i + 1) {
+ toValue = toValue / 1024;
+ unit = units[i];
+ }
+ return `${toValue.toLocaleString(undefined, { maximumFractionDigits: 2 })}
${unit}`;
+ }
+
+ static formatEphemeralStorageBytes(value: number | string): string {
+ const units: readonly string[] = ['KB', 'MB', 'GB', 'TB', 'PB', 'EB'];
+ let unit: string = 'bytes';
Review Comment:
This should be just a `B` for consistency with the other units.
##########
src/app/services/scheduler/scheduler.service.spec.ts:
##########
@@ -20,25 +20,84 @@ import {HttpClientTestingModule} from
'@angular/common/http/testing';
import {TestBed} from '@angular/core/testing';
import {EnvconfigService} from '@app/services/envconfig/envconfig.service';
import {MockEnvconfigService} from '@app/testing/mocks';
-import {configureTestSuite} from 'ng-bullet';
import {SchedulerService} from './scheduler.service';
+import {SchedulerResourceInfo} from '@app/models/resource-info.model';
+import {NOT_AVAILABLE} from '@app/utils/constants';
describe('SchedulerService', () => {
let service: SchedulerService;
- configureTestSuite(() => {
+ beforeEach(() => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [SchedulerService, { provide: EnvconfigService, useValue:
MockEnvconfigService }],
});
- });
-
- beforeEach(() => {
service = TestBed.inject(SchedulerService);
});
it('should create the service', () => {
expect(service).toBeTruthy();
+
+ });
+
+ it('should format SchedulerResourceInfo correctly', () => {
+ type TestCase = {
+ description: string;
+ schedulerResourceInfo: SchedulerResourceInfo;
+ expected: string;
+ };
+
+ const testCases: TestCase[] = [
+ {
+ description: 'test simple resourceInfo',
+ schedulerResourceInfo: {
+ memory: 1024,
+ vcore: 2,
Review Comment:
For consistency should this not be `'memory'` and `'vcore'` like it is for
`'pods'` etc.
##########
src/app/utils/common.util.ts:
##########
@@ -30,34 +30,55 @@ export class CommonUtil {
return uniqid;
}
- static formatBytes(value: number | string): string {
- const units: readonly string[] = ['kB', 'MB', 'GB', 'TB', 'PB'];
+ static formatMemoryBytes(value: number | string): string {
+ const units: readonly string[] = ['KiB', 'MiB', 'GiB', 'TiB', 'PiB',
'EiB'];
let unit: string = 'bytes';
- let toValue = +value
+ let toValue = +value;
+ for (let i = 0, unitslen = units.length; toValue / 1024 >= 1 && i <
unitslen;i = i + 1) {
+ toValue = toValue / 1024;
+ unit = units[i];
+ }
+ return `${toValue.toLocaleString(undefined, { maximumFractionDigits: 2 })}
${unit}`;
+ }
+
+ static formatEphemeralStorageBytes(value: number | string): string {
+ const units: readonly string[] = ['KB', 'MB', 'GB', 'TB', 'PB', 'EB'];
Review Comment:
The official kilo byte unit is `kB` in contrast to the `KiB` unit for the
binary version:
Binary prefixes: https://physics.nist.gov/cuu/Units/binary.html
SI prefixes: https://www.nist.gov/pml/owm/metric-si-prefixes
##########
src/app/services/scheduler/scheduler.service.ts:
##########
@@ -325,48 +325,66 @@ export class SchedulerService {
}
private formatResource(resource: SchedulerResourceInfo): string {
- const formatted = [];
-
- if (resource && resource.memory !== undefined) {
- formatted.push(`Memory: ${CommonUtil.formatBytes(resource.memory)}`);
- } else {
- formatted.push(`Memory: ${NOT_AVAILABLE}`);
- }
-
- if (resource && resource.vcore !== undefined) {
- formatted.push(`CPU: ${CommonUtil.formatCount(resource.vcore)}`);
- } else {
- formatted.push(`CPU: ${NOT_AVAILABLE}`);
- }
+ const formatted: string[] = [];
+ if (resource) {
+ // Object.keys() didn't guarantee the order of keys, sort it before
iterate.
+ Object.keys(resource).sort(this.resourcesCompareFn).forEach((key) => {
+ let value = resource[key];
+ let formattedKey = key;
+ let formattedValue = NOT_AVAILABLE;
- if (resource){
- Object.keys(resource).forEach((key) => {
switch(key){
case "memory":
- case "vcore":{
+ formattedKey = "Memory";
+ formattedValue = CommonUtil.formatMemoryBytes(value);
+ break;
+ case "vcore":
+ formattedKey = "CPU";
+ formattedValue = CommonUtil.formatCpuCore(value);
break;
- }
- case "ephemeral-storage":{
- if (resource[`ephemeral-storage`] == 0) {
- formatted.push(`ephemeral-storage: ${NOT_AVAILABLE}`);
- }else{
- formatted.push(`ephemeral-storage:
${CommonUtil.formatBytes(resource[key])}`);
+ case "ephemeral-storage":
+ if (value !== 0){ // if value is 0, show NOT_AVAILABLE
+ formattedValue = CommonUtil.formatEphemeralStorageBytes(value);
}
break;
- }
- default:{
- if (resource[key] == 0) {
- formatted.push(`${key}: ${NOT_AVAILABLE}`);
- }else{
- formatted.push(`${key}:
${CommonUtil.formatOtherResource(resource[key])}`);
+ default:
+ if (value !== 0){ // if value is 0, show NOT_AVAILABLE
+ if (key.startsWith('hugepages-')) {
+ formattedValue = CommonUtil.formatMemoryBytes(value);
+ } else{
+ formattedValue = CommonUtil.formatOtherResource(value);
+ }
}
break;
- }
- }
+ }
+ // }
+ formatted.push(`${formattedKey}: ${formattedValue}`);
});
}
-
+
+ // Set CPU/Memory as NOT_AVAILABLE if it was not in Object.keys() or
resource is undefined.
+ if (!resource || resource.vcore === undefined)
+ formatted.unshift(`CPU: ${NOT_AVAILABLE}`);
+ if (!resource || resource.memory === undefined)
+ formatted.unshift(`Memory: ${NOT_AVAILABLE}`);
Review Comment:
See comment added I don't think we should do this.
--
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]
