yangwwei commented on a change in pull request #216:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/216#discussion_r562401523
##########
File path: pkg/scheduler/partition.go
##########
@@ -54,13 +54,13 @@ type PartitionContext struct {
allocations map[string]*objects.Allocation // allocations
placementManager *placement.AppPlacementManager // placement
manager for this partition
partitionManager *partitionManager // manager for
this partition
- stateMachine *fsm.FSM // the state of
the partition for scheduling
- stateTime time.Time // last time the
state was updated (needed for cleanup)
+ StateMachine *fsm.FSM // the state of
the partition for scheduling
+ StateTime time.Time // last time the
state was updated (needed for cleanup)
isPreemptable bool // can
allocations be preempted
rules *[]configs.PlacementRule // placement
rules to be loaded by the scheduler
userGroupCache *security.UserGroupCache // user cache
per partition
totalPartitionResource *resources.Resource // Total node
resources
- nodeSortingPolicy *policies.NodeSortingPolicy // Global Node
Sorting Policies
+ NodeSortingPolicy *policies.NodeSortingPolicy // Global Node
Sorting Policies
Review comment:
Do not expose this field as well. We can introduce a function to return
the sorting policy instead.
##########
File path: pkg/webservice/handlers.go
##########
@@ -521,3 +521,39 @@ func updateConfiguration(conf string) (string, error) {
}
return "", fmt.Errorf("config plugin not found")
}
+
+func getPartitions(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+
+ var partitionsInfo []*dao.PartitionInfo
+ lists := schedulerContext.GetPartitionMapClone()
+ for _, partitionContext := range lists {
+ partitionInfo := &dao.PartitionInfo{}
+ partitionInfo.Name = partitionContext.Name
+ partitionInfo.State = partitionContext.StateMachine.Current()
Review comment:
It is not a good idea to expose `StateMachine` and `StateTime`, they are
pretty internal objects.
Instead, we can create a `GetCurrentState()` function that returns a string.
Same for the stateTime.
##########
File path: pkg/webservice/dao/partition_info.go
##########
@@ -24,12 +24,29 @@ type PartitionDAOInfo struct {
Queues QueueDAOInfo `json:"queues"`
}
+type PartitionInfo struct {
+ Name string `json:"name"`
+ Capacity PartitionCapacity `json:"capacity"`
+ NodeSortingPolicy string `json:"nodeSortingPolicy"`
+ Applications Applications `json:"applications"`
+ State string `json:"state"`
+ LastStateTransitionTime string
`json:"lastStateTransitionTime"`
+}
+
type PartitionCapacity struct {
Capacity string `json:"capacity"`
- UsedCapacity string `json:"usedcapacity"`
+ UsedCapacity string `json:"usedCapacity"`
}
type NodeInfo struct {
NodeID string `json:"nodeId"`
Capability string `json:"capability"`
}
+
+type Applications struct {
+ Total int `json:"total"`
+ Running int `json:"running"`
+ Pending int `json:"pending"`
+ Completed int `json:"completed"`
+ Failed int `json:"failed"`
Review comment:
My earlier comment is to remove hardcoded state names.
Because if we modified app state in the scheduler package, and most likely
we are going to do so. It is going to break this. And I see we are doing some
formatting like:
```
applicationsInfo.Running = applicationsState["Running"]
applicationsInfo.Pending = applicationsState["Waiting"] +
applicationsState["Accepted"] + applicationsState["Starting"] +
applicationsState["New"]
applicationsInfo.Completed = applicationsState["Completed"]
applicationsInfo.Failed = applicationsState["Killed"] +
applicationsState["Rejected"]
applicationsInfo.Total = applicationsInfo.Running + applicationsInfo.Pending
+ applicationsInfo.Completed + applicationsInfo.Failed
```
I'd rather stay with the initial values. Because we are documenting all the
app states carefully, see
http://yunikorn.apache.org/docs/next/design/scheduler_object_states#application-state.
We need to keep the REST API response align with the doc. So I think returning
its original map plus a total number is enough. Does that make sense to you?
##########
File path: pkg/scheduler/partition.go
##########
@@ -54,13 +54,13 @@ type PartitionContext struct {
allocations map[string]*objects.Allocation // allocations
placementManager *placement.AppPlacementManager // placement
manager for this partition
partitionManager *partitionManager // manager for
this partition
- stateMachine *fsm.FSM // the state of
the partition for scheduling
- stateTime time.Time // last time the
state was updated (needed for cleanup)
Review comment:
See my comments later for this, we should not expose the internal
objects out of its package.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
