[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #507: [YUNIKORN-1385] To provide visibility to application's aggregated res…

["zhuqi-lucas (via GitHub)" <gi...@apache.org>]

zhuqi-lucas commented on code in PR #507:
URL: https://github.com/apache/yunikorn-core/pull/507#discussion_r1113821964


##########
pkg/metrics/init.go:
##########
@@ -229,3 +236,7 @@ func formatMetricName(metricName string) string {
 	}
 	return string(newBytes)
 }
+
+func (m *Metrics) LogAppSummary(appSummary string) {
+	log.Println(AppSummaryHeader + appSummary)
+}

Review Comment:
   Should add a newline at the end of file.



##########
pkg/scheduler/objects/application_test.go:
##########
@@ -1061,6 +1064,125 @@ func TestCompleted(t *testing.T) {
 	assert.Equal(t, log[3].ApplicationState, Expired.String())
 }
 
+func assertResourceUsage(t *testing.T, appSummary string, memorySeconds int64, vcoresSecconds int64) {
+	var jsonMap map[string]interface{}
+	var resource interface{}
+	var detailedResource map[string]interface{}
+
+	json.Unmarshal([]byte(appSummary), &jsonMap)
+
+	resource = jsonMap["resourceUsage"]
+
+	resourceUsageMap := resource.(map[string]interface{})
+	//fmt.Print("resourceMap: ")

Review Comment:
   Remove debug print.



##########
pkg/scheduler/objects/application_test.go:
##########
@@ -1061,6 +1064,125 @@ func TestCompleted(t *testing.T) {
 	assert.Equal(t, log[3].ApplicationState, Expired.String())
 }
 
+func assertResourceUsage(t *testing.T, appSummary string, memorySeconds int64, vcoresSecconds int64) {
+	var jsonMap map[string]interface{}
+	var resource interface{}
+	var detailedResource map[string]interface{}
+
+	json.Unmarshal([]byte(appSummary), &jsonMap)
+
+	resource = jsonMap["resourceUsage"]
+
+	resourceUsageMap := resource.(map[string]interface{})
+	//fmt.Print("resourceMap: ")
+	//fmt.Println(resourceUsageMap)
+
+	itypeResource, ok := resourceUsageMap["itype-1"]
+	if !ok {
+		assert.Assert(t, memorySeconds == -1 || vcoresSecconds == -1, "no resource usage")
+		return
+	}
+
+	//fmt.Print("itypeResource: ")

Review Comment:
   Remove debug print.



##########
pkg/common/resources/resources.go:
##########
@@ -44,6 +46,55 @@ func (q Quantity) string() string {
 	return strconv.FormatInt(int64(q), 10)
 }
 
+// Util struct to keep track of application resource usage
+type UsedResourceTracker struct {
+	// Two level map for aggregated resource usage
+	// With instance type being the top level key, the mapped value is a map:
+	//   resource type (CPU, memory etc) -> the aggregated used time (in seconds) of the resource type
+	//
+	UsedResourceMap map[string]map[string]int64

Review Comment:
   If we have a time to clean up the UsedResourceMap, if it will cause resource leak.



##########
pkg/scheduler/objects/application.go:
##########
@@ -93,6 +97,7 @@ type Application struct {
 	execTimeout          time.Duration               // execTimeout for the application run
 	placeholderTimer     *time.Timer                 // placeholder replace timer
 	gangSchedulingStyle  string                      // gang scheduling style can be hard (after timeout we fail the application), or soft (after timeeout we schedule it as a normal application)
+	startTime            time.Time                   // the time that the application starts running. Default is zero.

Review Comment:
   One question, when we are starting state the app for gang scheduler enabled, it means the gang scheduler placeholder allocations are all allocated, if these placeholder allocations need to calculate the resource usage, because before the startTime here, the gang scheduler allocations are all bound in k8s shim side.
   And if the gang scheduler timeout, if the placeholder allocations need to calculate even the app failed, because some placeholder allocations have been occupied resources for some time, and the default timeout is 15 mins, it's not a short time for resources occupied.
   
   cc @wilfred-s @craigcondit 



##########
pkg/scheduler/objects/node.go:
##########
@@ -117,6 +121,15 @@ func (sn *Node) GetAttribute(key string) string {
 	return sn.attributes[key]
 }
 
+// Get inst type from attributes passed to core from shim.
+func (sn *Node) GetInstType() string {
+	itype := sn.GetAttribute(YunikornInstanceType)

Review Comment:
   Change after instanceType merged.



-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org