[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #216: YUNIKORN-414: Add partitions REST API to fetch list of partitions

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org