-
Notifications
You must be signed in to change notification settings - Fork 549
Conversation
} | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add a yarn dependency to rest server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. the desired behavior is: if yarn is available, it works. if yarn is unavailable, the system says it doesn't support the feature yet.
In reply to: 239478932 [](ancestors = 239478932)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gerhut @fanyangCS
I will move the validation somewhere to remove the dependency. But I think we need clarify vc
firstly, it will affect our API design.
If we consider vc
as the same concept of hadoop queue
, moving the check to vc
related API sounds reasonable, because only yarn provider this feature.
But if vc
is designed as a abstract interface, hadoop queue
is only one implementation. I think we need refactor our vc structure, to support other implementation. At least, some API coupling with hadoop queue
shoulded be add to our doc.
src/rest-server/src/models/vc.js
Outdated
function traverse(queueInfo, queueDict) { | ||
if (queueInfo.type === 'capacitySchedulerLeafQueueInfo') { | ||
queueDict[queueInfo.queueName] = { | ||
capacity: queueInfo.absoluteCapacity, | ||
maxCapacity: queueInfo.absoluteMaxCapacity, | ||
capacity: parseInt(queueInfo.absoluteCapacity), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Number()
instead of parseInt()
to avoid number base issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve by math.round
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built-in Math.round
is enough
if (err) { | ||
return callback(err); | ||
} else { | ||
let defaultQuotaIfUpdated = vcList['default']['capacity'] + vcList[vcName]['capacity']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make sure about these 2 values will be Number? +
will be treated as string concat when one of these is string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this two value comes from YARN, they are both number.
2 similar comments
Checkin firstly to help frontend test, will remove this deps in other pr |
* VC update API doc (#1816) * refine api doc * refine doc * refine * refine * [VC update] rest-server (#1831) * change scheduler storage to zookeeper * add queue update * fix * add vc update api * add yarn health check, refine error * add api doc * refine api doc * refine api doc * update yarn response * add unit test * unit test * replace parseint with math.round, fix unit test * replace mathjs with build-in Math * vc update (#1974) * Update yarnContainerScript.mustache add entrypoint="" * add design for vc update * refine design of vc update * add description of /api/v1/:username/virtualClusters * remove design of vc_update to dir of webportal * refine design of vc * VC function development * vc style modification * vc list style modification * Modify the prompt * repair bug * remove design of vc_update and refine annotation * refine var name * Change eslint error * Optimize the code according to pr * Modified read-only
add 3 vc related api:
PUT virtual-clusters/:vcName
add or update a vc quota
DELETE virtual-clusters/:vcName
delete a vc
PUT virtual-clusters/:vcName/status
change vc status