You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/02/18 04:13:24 UTC

[GitHub] [incubator-yunikorn-core] wilfred-s opened a new pull request #251: [YUNIKORN-505] placeholder allocate tests

wilfred-s opened a new pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251


   Additional tests for placeholder allocation. Introducing a new plugin
   for mocking the predicate plugin.
   Miscellaneous Logging fixes to help troubleshoot failures and progress of
   allocations.


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583256581



##########
File path: pkg/scheduler/context.go
##########
@@ -626,27 +656,18 @@ func (cc *ClusterContext) processAsks(request *si.UpdateRequest) {
 			continue
 		}
 
-		// if app info doesn't exist, reject the request
-		app := partition.getApplication(siAsk.ApplicationID)
-		if app == nil {
-			msg := fmt.Sprintf("Failed to find application %s, for allocation %s", siAsk.ApplicationID, siAsk.AllocationKey)
-			log.Logger().Info(msg)
-			rejectedAsks = append(rejectedAsks,

Review comment:
       This has all been folded into one call. Instead of first checking the app on the partition and then calling the app directly we call the add on the partition which calls the add on the app. This brings it inline with the other checks we do and the siAsk is the loop variable and has not changed. The error still exists and still gets [returned](https://github.com/apache/incubator-yunikorn-core/pull/251/files#diff-e09ef6f5c0e4efd327afd240d335188fdb9d758af225dc958f0687257c75333eR1200-R1202) that returned error triggers the reject. It is now just one call.




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#issuecomment-781037792


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=h1) Report
   > Merging [#251](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=desc) (9bb55ff) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.84%`.
   > The diff coverage is `63.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #251      +/-   ##
   ==========================================
   + Coverage   63.46%   64.31%   +0.84%     
   ==========================================
     Files          60       60              
     Lines        5220     5299      +79     
   ==========================================
   + Hits         3313     3408      +95     
   + Misses       1747     1735      -12     
   + Partials      160      156       -4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.86% <0.00%> (-0.40%)` | :arrow_down: |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `69.28% <63.41%> (+3.23%)` | :arrow_up: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `53.66% <71.42%> (+4.75%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `56.73% <91.30%> (+2.59%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.22% <94.28%> (+0.19%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/allocation\_ask.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb25fYXNrLmdv) | `93.87% <100.00%> (ø)` | |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=footer). Last update [4c53c50...8250110](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583245300



##########
File path: pkg/scheduler/partition.go
##########
@@ -585,10 +580,6 @@ func (pc *PartitionContext) AddNode(node *objects.Node, existingAllocations []*o
 
 	// Node is added update the metrics
 	metrics.GetSchedulerMetrics().IncActiveNodes()
-	log.Logger().Info("added node to partition",
-		zap.String("partitionName", pc.Name),
-		zap.String("nodeID", node.NodeID),
-		zap.String("partitionResource", pc.totalPartitionResource.String()))

Review comment:
       this has been moved to an INFO level message in the partition to standardise the way we log. All successes and fails are logged there. Corresponding message in [partition](https://github.com/apache/incubator-yunikorn-core/pull/251/files#diff-a6bde2f6900fb05707fa68db3365f08a79642f39ea629ad5b489d05d3cacc7b3R497-R499)




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#issuecomment-781037792


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=h1) Report
   > Merging [#251](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=desc) (8250110) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.78%`.
   > The diff coverage is `65.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #251      +/-   ##
   ==========================================
   + Coverage   63.46%   64.25%   +0.78%     
   ==========================================
     Files          60       60              
     Lines        5220     5324     +104     
   ==========================================
   + Hits         3313     3421     +108     
   + Misses       1747     1746       -1     
   + Partials      160      157       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.86% <0.00%> (-0.40%)` | :arrow_down: |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `68.99% <60.00%> (+2.94%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `57.00% <91.66%> (+2.86%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.22% <94.28%> (+0.19%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/allocation\_ask.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb25fYXNrLmdv) | `93.87% <100.00%> (ø)` | |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `53.19% <100.00%> (+4.28%)` | :arrow_up: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.83% <100.00%> (+0.05%)` | :arrow_up: |
   | ... and [4 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=footer). Last update [4c53c50...8250110](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s closed pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251


   


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583266333



##########
File path: pkg/scheduler/partition.go
##########
@@ -1185,18 +1180,29 @@ func (pc *PartitionContext) removeAllocation(release *si.AllocationRelease) ([]*
 // NOTE: this is a lock free call. It must NOT be called holding the PartitionContext lock.
 func (pc *PartitionContext) removeAllocationAsk(appID string, allocationKey string) {
 	app := pc.getApplication(appID)
-	if app != nil {
-		// remove the allocation asks from the app
-		reservedAsks := app.RemoveAllocationAsk(allocationKey)
-		log.Logger().Info("release allocation ask",
+	if app == nil {
+		log.Logger().Info("Invalid ask release requested by shim",
 			zap.String("allocation", allocationKey),
-			zap.String("appID", appID),
-			zap.Int("reservedAskReleased", reservedAsks))
-		// update the partition if the asks were reserved (clean up)
-		if reservedAsks != 0 {
-			pc.unReserveCount(appID, reservedAsks)
-		}
+			zap.String("appID", appID))
+		return
+	}
+	// remove the allocation asks from the app
+	reservedAsks := app.RemoveAllocationAsk(allocationKey)
+	// update the partition if the asks were reserved (clean up)
+	if reservedAsks != 0 {
+		pc.unReserveCount(appID, reservedAsks)
+	}
+}
+
+// Remove the allocation Ask from the specified application
+// NOTE: this is a lock free call. It must NOT be called holding the PartitionContext lock.
+func (pc *PartitionContext) addAllocationAsk(siAsk *si.AllocationAsk) error {
+	app := pc.getApplication(siAsk.ApplicationID)
+	if app == nil {
+		return fmt.Errorf("failed to find application %s, for allocation ask %s", siAsk.ApplicationID, siAsk.AllocationKey)
 	}
+	// remove the allocation asks from the app

Review comment:
       done

##########
File path: pkg/scheduler/partition.go
##########
@@ -1185,18 +1180,29 @@ func (pc *PartitionContext) removeAllocation(release *si.AllocationRelease) ([]*
 // NOTE: this is a lock free call. It must NOT be called holding the PartitionContext lock.
 func (pc *PartitionContext) removeAllocationAsk(appID string, allocationKey string) {
 	app := pc.getApplication(appID)
-	if app != nil {
-		// remove the allocation asks from the app
-		reservedAsks := app.RemoveAllocationAsk(allocationKey)
-		log.Logger().Info("release allocation ask",
+	if app == nil {
+		log.Logger().Info("Invalid ask release requested by shim",
 			zap.String("allocation", allocationKey),
-			zap.String("appID", appID),
-			zap.Int("reservedAskReleased", reservedAsks))
-		// update the partition if the asks were reserved (clean up)
-		if reservedAsks != 0 {
-			pc.unReserveCount(appID, reservedAsks)
-		}
+			zap.String("appID", appID))
+		return
+	}
+	// remove the allocation asks from the app
+	reservedAsks := app.RemoveAllocationAsk(allocationKey)
+	// update the partition if the asks were reserved (clean up)
+	if reservedAsks != 0 {
+		pc.unReserveCount(appID, reservedAsks)
+	}
+}
+
+// Remove the allocation Ask from the specified application

Review comment:
       done




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#issuecomment-781037792


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=h1) Report
   > Merging [#251](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=desc) (504c7f5) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.48%`.
   > The diff coverage is `60.98%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #251      +/-   ##
   ==========================================
   + Coverage   63.46%   63.95%   +0.48%     
   ==========================================
     Files          60       60              
     Lines        5220     5296      +76     
   ==========================================
   + Hits         3313     3387      +74     
   - Misses       1747     1752       +5     
   + Partials      160      157       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.86% <0.00%> (-0.40%)` | :arrow_down: |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `50.53% <33.33%> (+1.63%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `69.28% <63.41%> (+3.23%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `56.73% <91.30%> (+2.59%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.22% <94.28%> (+0.19%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/allocation\_ask.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb25fYXNrLmdv) | `93.87% <100.00%> (ø)` | |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=footer). Last update [4c53c50...9bb55ff](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583248064



##########
File path: pkg/scheduler/partition.go
##########
@@ -380,9 +379,6 @@ func (pc *PartitionContext) removeApplication(appID string) []*objects.Allocatio
 			}
 		}
 	}
-	log.Logger().Debug("application removed from the scheduler",
-		zap.String("queue", queueName),
-		zap.String("applicationID", appID))

Review comment:
       We have more logs now, as part of the change we log this in the context at an INFO level to make sure we log all possible failure cases for adds and the removed entry has been [moved](https://github.com/apache/incubator-yunikorn-core/pull/251/files#diff-a6bde2f6900fb05707fa68db3365f08a79642f39ea629ad5b489d05d3cacc7b3R497-R499) add and remove log the same way now




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583245300



##########
File path: pkg/scheduler/partition.go
##########
@@ -585,10 +580,6 @@ func (pc *PartitionContext) AddNode(node *objects.Node, existingAllocations []*o
 
 	// Node is added update the metrics
 	metrics.GetSchedulerMetrics().IncActiveNodes()
-	log.Logger().Info("added node to partition",
-		zap.String("partitionName", pc.Name),
-		zap.String("nodeID", node.NodeID),
-		zap.String("partitionResource", pc.totalPartitionResource.String()))

Review comment:
       this has been moved to an INFO level message in the partition to standardise the way we log. All successes and fails are logged there. Corresponding message in [partition](https://github.com/apache/incubator-yunikorn-core/pull/251/files#diff-a6bde2f6900fb05707fa68db3365f08a79642f39ea629ad5b489d05d3cacc7b3R605-R607)




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r582764887



##########
File path: pkg/scheduler/partition.go
##########
@@ -1185,18 +1180,29 @@ func (pc *PartitionContext) removeAllocation(release *si.AllocationRelease) ([]*
 // NOTE: this is a lock free call. It must NOT be called holding the PartitionContext lock.
 func (pc *PartitionContext) removeAllocationAsk(appID string, allocationKey string) {
 	app := pc.getApplication(appID)
-	if app != nil {
-		// remove the allocation asks from the app
-		reservedAsks := app.RemoveAllocationAsk(allocationKey)
-		log.Logger().Info("release allocation ask",
+	if app == nil {
+		log.Logger().Info("Invalid ask release requested by shim",
 			zap.String("allocation", allocationKey),
-			zap.String("appID", appID),
-			zap.Int("reservedAskReleased", reservedAsks))
-		// update the partition if the asks were reserved (clean up)
-		if reservedAsks != 0 {
-			pc.unReserveCount(appID, reservedAsks)
-		}
+			zap.String("appID", appID))
+		return
+	}
+	// remove the allocation asks from the app
+	reservedAsks := app.RemoveAllocationAsk(allocationKey)
+	// update the partition if the asks were reserved (clean up)
+	if reservedAsks != 0 {
+		pc.unReserveCount(appID, reservedAsks)
+	}
+}
+
+// Remove the allocation Ask from the specified application

Review comment:
       Please fix the comment, since here we are adding Allocation ask not removing it.

##########
File path: pkg/scheduler/partition.go
##########
@@ -380,9 +379,6 @@ func (pc *PartitionContext) removeApplication(appID string) []*objects.Allocatio
 			}
 		}
 	}
-	log.Logger().Debug("application removed from the scheduler",
-		zap.String("queue", queueName),
-		zap.String("applicationID", appID))

Review comment:
       I think this log message can be useful. Right now we have no logs when we remove an application

##########
File path: pkg/scheduler/context.go
##########
@@ -626,27 +656,18 @@ func (cc *ClusterContext) processAsks(request *si.UpdateRequest) {
 			continue
 		}
 
-		// if app info doesn't exist, reject the request
-		app := partition.getApplication(siAsk.ApplicationID)
-		if app == nil {
-			msg := fmt.Sprintf("Failed to find application %s, for allocation %s", siAsk.ApplicationID, siAsk.AllocationKey)
-			log.Logger().Info(msg)
-			rejectedAsks = append(rejectedAsks,

Review comment:
       after moving this check to the partitionContext, if this check will fail, the rejectedAsks list will not have the actual one.

##########
File path: pkg/scheduler/partition_test.go
##########
@@ -1221,3 +1222,237 @@ func TestUpdateNode(t *testing.T) {
 		t.Errorf("Expected partition resource %s, doesn't match with actual partition resource %s", expectedRes, partition.GetTotalPartitionResource())
 	}
 }
+
+func TestAddTGApplication(t *testing.T) {
+	limit := map[string]string{"first": "1"}
+	partition, err := newLimitedPartition(limit)
+	assert.NilError(t, err, "partition create failed")
+	// add a app with TG that does not fit in the queue
+	var tgRes *resources.Resource
+	tgRes, err = resources.NewResourceFromConf(map[string]string{"first": "10"})
+	assert.NilError(t, err, "failed to create resource")
+	app := newApplicationTG(appID1, "default", "root.limited", tgRes)
+	err = partition.AddApplication(app)
+	if err == nil {
+		t.Error("app-1 should be rejected due to TG request")
+	}
+
+	limit = map[string]string{"first": "100"}
+	partition, err = newLimitedPartition(limit)
+	assert.NilError(t, err, "partition create failed")
+	err = partition.AddApplication(app)
+	assert.NilError(t, err, "app-1 should have been added to the partition")

Review comment:
       Besides checking for error please check if the application was added to the application list as well.

##########
File path: pkg/scheduler/partition.go
##########
@@ -585,10 +580,6 @@ func (pc *PartitionContext) AddNode(node *objects.Node, existingAllocations []*o
 
 	// Node is added update the metrics
 	metrics.GetSchedulerMetrics().IncActiveNodes()
-	log.Logger().Info("added node to partition",
-		zap.String("partitionName", pc.Name),
-		zap.String("nodeID", node.NodeID),
-		zap.String("partitionResource", pc.totalPartitionResource.String()))

Review comment:
       This one can be helpful as well, I think. Maybe at debug level is enough, but I don't think we should remove it.

##########
File path: pkg/scheduler/partition.go
##########
@@ -1185,18 +1180,29 @@ func (pc *PartitionContext) removeAllocation(release *si.AllocationRelease) ([]*
 // NOTE: this is a lock free call. It must NOT be called holding the PartitionContext lock.
 func (pc *PartitionContext) removeAllocationAsk(appID string, allocationKey string) {
 	app := pc.getApplication(appID)
-	if app != nil {
-		// remove the allocation asks from the app
-		reservedAsks := app.RemoveAllocationAsk(allocationKey)
-		log.Logger().Info("release allocation ask",
+	if app == nil {
+		log.Logger().Info("Invalid ask release requested by shim",
 			zap.String("allocation", allocationKey),
-			zap.String("appID", appID),
-			zap.Int("reservedAskReleased", reservedAsks))
-		// update the partition if the asks were reserved (clean up)
-		if reservedAsks != 0 {
-			pc.unReserveCount(appID, reservedAsks)
-		}
+			zap.String("appID", appID))
+		return
+	}
+	// remove the allocation asks from the app
+	reservedAsks := app.RemoveAllocationAsk(allocationKey)
+	// update the partition if the asks were reserved (clean up)
+	if reservedAsks != 0 {
+		pc.unReserveCount(appID, reservedAsks)
+	}
+}
+
+// Remove the allocation Ask from the specified application
+// NOTE: this is a lock free call. It must NOT be called holding the PartitionContext lock.
+func (pc *PartitionContext) addAllocationAsk(siAsk *si.AllocationAsk) error {
+	app := pc.getApplication(siAsk.ApplicationID)
+	if app == nil {
+		return fmt.Errorf("failed to find application %s, for allocation ask %s", siAsk.ApplicationID, siAsk.AllocationKey)
 	}
+	// remove the allocation asks from the app

Review comment:
       please fix the comment




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583266255



##########
File path: pkg/scheduler/partition_test.go
##########
@@ -1221,3 +1222,237 @@ func TestUpdateNode(t *testing.T) {
 		t.Errorf("Expected partition resource %s, doesn't match with actual partition resource %s", expectedRes, partition.GetTotalPartitionResource())
 	}
 }
+
+func TestAddTGApplication(t *testing.T) {
+	limit := map[string]string{"first": "1"}
+	partition, err := newLimitedPartition(limit)
+	assert.NilError(t, err, "partition create failed")
+	// add a app with TG that does not fit in the queue
+	var tgRes *resources.Resource
+	tgRes, err = resources.NewResourceFromConf(map[string]string{"first": "10"})
+	assert.NilError(t, err, "failed to create resource")
+	app := newApplicationTG(appID1, "default", "root.limited", tgRes)
+	err = partition.AddApplication(app)
+	if err == nil {
+		t.Error("app-1 should be rejected due to TG request")
+	}
+
+	limit = map[string]string{"first": "100"}
+	partition, err = newLimitedPartition(limit)
+	assert.NilError(t, err, "partition create failed")
+	err = partition.AddApplication(app)
+	assert.NilError(t, err, "app-1 should have been added to the partition")

Review comment:
       done




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#issuecomment-781037792


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=h1) Report
   > Merging [#251](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=desc) (504c7f5) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.48%`.
   > The diff coverage is `60.98%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #251      +/-   ##
   ==========================================
   + Coverage   63.46%   63.95%   +0.48%     
   ==========================================
     Files          60       60              
     Lines        5220     5296      +76     
   ==========================================
   + Hits         3313     3387      +74     
   - Misses       1747     1752       +5     
   + Partials      160      157       -3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.86% <0.00%> (-0.40%)` | :arrow_down: |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `50.53% <33.33%> (+1.63%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `69.28% <63.41%> (+3.23%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `56.73% <91.30%> (+2.59%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.22% <94.28%> (+0.19%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/allocation\_ask.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb25fYXNrLmdv) | `93.87% <100.00%> (ø)` | |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   | ... and [3 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=footer). Last update [4c53c50...504c7f5](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583248064



##########
File path: pkg/scheduler/partition.go
##########
@@ -380,9 +379,6 @@ func (pc *PartitionContext) removeApplication(appID string) []*objects.Allocatio
 			}
 		}
 	}
-	log.Logger().Debug("application removed from the scheduler",
-		zap.String("queue", queueName),
-		zap.String("applicationID", appID))

Review comment:
       We have more logs now, as part of the change we log this in the context at an INFO level to make sure we log all possible failure cases [moved entry](https://github.com/apache/incubator-yunikorn-core/pull/251/files#diff-a6bde2f6900fb05707fa68db3365f08a79642f39ea629ad5b489d05d3cacc7b3R497-R499)




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583256581



##########
File path: pkg/scheduler/context.go
##########
@@ -626,27 +656,18 @@ func (cc *ClusterContext) processAsks(request *si.UpdateRequest) {
 			continue
 		}
 
-		// if app info doesn't exist, reject the request
-		app := partition.getApplication(siAsk.ApplicationID)
-		if app == nil {
-			msg := fmt.Sprintf("Failed to find application %s, for allocation %s", siAsk.ApplicationID, siAsk.AllocationKey)
-			log.Logger().Info(msg)
-			rejectedAsks = append(rejectedAsks,

Review comment:
       This has all been folded into one call. Instead of first checking the app on the partition and then calling the app directly we call the add on the partition which calls the add on the app. This brings it inline with the other checks we do and the siAsk is the loop variable and has not changed. The error still exists and still gets [returned](https://github.com/apache/incubator-yunikorn-core/pull/251/files)




----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#issuecomment-781037792


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=h1) Report
   > Merging [#251](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=desc) (9bb55ff) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.84%`.
   > The diff coverage is `63.36%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #251      +/-   ##
   ==========================================
   + Coverage   63.46%   64.31%   +0.84%     
   ==========================================
     Files          60       60              
     Lines        5220     5299      +79     
   ==========================================
   + Hits         3313     3408      +95     
   + Misses       1747     1735      -12     
   + Partials      160      156       -4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.86% <0.00%> (-0.40%)` | :arrow_down: |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `69.28% <63.41%> (+3.23%)` | :arrow_up: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `53.66% <71.42%> (+4.75%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `56.73% <91.30%> (+2.59%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.22% <94.28%> (+0.19%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/allocation\_ask.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb25fYXNrLmdv) | `93.87% <100.00%> (ø)` | |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=footer). Last update [4c53c50...9bb55ff](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/251?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #251: [YUNIKORN-505] placeholder allocate tests

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #251:
URL: https://github.com/apache/incubator-yunikorn-core/pull/251#discussion_r583245300



##########
File path: pkg/scheduler/partition.go
##########
@@ -585,10 +580,6 @@ func (pc *PartitionContext) AddNode(node *objects.Node, existingAllocations []*o
 
 	// Node is added update the metrics
 	metrics.GetSchedulerMetrics().IncActiveNodes()
-	log.Logger().Info("added node to partition",
-		zap.String("partitionName", pc.Name),
-		zap.String("nodeID", node.NodeID),
-		zap.String("partitionResource", pc.totalPartitionResource.String()))

Review comment:
       this has been moved to an INFO level message in the partition to standardise the way we log.




----------------------------------------------------------------
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