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 2020/12/16 16:59:25 UTC

[GitHub] [incubator-yunikorn-core] manirajv06 opened a new pull request #231: YUNIKORN-418: Add "config" REST API

manirajv06 opened a new pull request #231:
URL: https://github.com/apache/incubator-yunikorn-core/pull/231


   First cut implementation. In addition, response middleware has been added to return error also in json format and used httptest package for unit testing the same.


----------------------------------------------------------------
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] manirajv06 commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/webservice.go
##########
@@ -58,14 +59,40 @@ func newRouter() *mux.Router {
 func loggingHandler(inner http.Handler, name string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
-
-		inner.ServeHTTP(w, r)
-
+		rw := &YResponseWriter{ResponseWriter: w, statusCode: -1}
+		inner.ServeHTTP(rw, r)
+		var body = string(rw.body)
+		if rw.statusCode != 200 && rw.statusCode != -1 {
+			errorInfo := dao.NewYAPIError(nil, rw.statusCode, body)

Review comment:
       Missed this file. Added 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] manirajv06 commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if queryParams.Get("dry_run") != "1" {
+		http.Error(w, "Invalid query params. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}

Review comment:
       Taken care. Filed a separate JIRA - https://issues.apache.org/jira/browse/YUNIKORN-515 to track doc/user guide changes.




----------------------------------------------------------------
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] manirajv06 commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/dao/error_info.go
##########
@@ -0,0 +1,33 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package dao
+
+type YAPIError struct {
+	StatusCode  int    `json:"status_code"`
+	Message     string `json:"message"`
+	Description string `json:"description"`
+}
+
+func NewYAPIError(err error, statusCode int, message string) *YAPIError {

Review comment:
       Taken care

##########
File path: pkg/webservice/routes.go
##########
@@ -102,7 +102,15 @@ var webRoutes = routes{
 		"Scheduler",
 		"PUT",
 		"/ws/v1/config",
-		updateConfig,
+		updateClusterConfig,
+	},
+
+	// endpoint to update the current conf

Review comment:
       Taken care




-- 
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] manirajv06 commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]

Review comment:
       Taken care




----------------------------------------------------------------
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 #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (f90d7e7) 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 `83.73%`.
   
   > :exclamation: Current head f90d7e7 differs from pull request most recent head f954680. Consider uploading reports for the commit f954680 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   63.95%   +0.48%     
   ==========================================
     Files          60       60              
     Lines        5220     5302      +82     
   ==========================================
   + Hits         3313     3391      +78     
   - Misses       1747     1749       +2     
   - Partials      160      162       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `6.28% <0.00%> (+0.01%)` | :arrow_up: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `48.73% <0.00%> (-0.18%)` | :arrow_down: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `65.94% <69.23%> (-0.11%)` | :arrow_down: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.20% <84.78%> (+4.07%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/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/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [5e8696c...f954680](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] manirajv06 commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if queryParams.Get("dry_run") != "1" {
+		http.Error(w, "Invalid query params. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}

Review comment:
       Taken care of error message. Filed a separate JIRA - https://issues.apache.org/jira/browse/YUNIKORN-515 to track doc/user guide changes.




----------------------------------------------------------------
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 #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)

Review comment:
       Pleas include in the error message exactly what parameter is missing. In our case "dry_run"

##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]

Review comment:
       Please don't ignore the returned value here and reuse it in the line 469:
   
   ```
   dryRun, dryRunExists := queryParams["dry_run"]
   ....
   if dryRun != "1" {
   ...
   ```

##########
File path: pkg/webservice/routes.go
##########
@@ -102,7 +102,15 @@ var webRoutes = routes{
 		"Scheduler",
 		"PUT",
 		"/ws/v1/config",
-		updateConfig,
+		updateClusterConfig,
+	},
+
+	// endpoint to update the current conf

Review comment:
       this is the endpoint to validate the config, not? the update is the previous one.

##########
File path: pkg/webservice/webservice.go
##########
@@ -58,14 +59,40 @@ func newRouter() *mux.Router {
 func loggingHandler(inner http.Handler, name string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
-
-		inner.ServeHTTP(w, r)
-
+		rw := &YResponseWriter{ResponseWriter: w, statusCode: -1}
+		inner.ServeHTTP(rw, r)
+		var body = string(rw.body)
+		if rw.statusCode != 200 && rw.statusCode != -1 {
+			errorInfo := dao.NewYAPIError(nil, rw.statusCode, body)

Review comment:
       This `NewYAPIError` method is not defined. This is the reason why Travis check is failing. 

##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if queryParams.Get("dry_run") != "1" {
+		http.Error(w, "Invalid query params. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}

Review comment:
       Right now we support only dry_run=1. I think we should document it and also include it in the error message.




----------------------------------------------------------------
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] manirajv06 commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/routes.go
##########
@@ -102,7 +102,15 @@ var webRoutes = routes{
 		"Scheduler",
 		"PUT",
 		"/ws/v1/config",
-		updateConfig,
+		updateClusterConfig,
+	},
+
+	// endpoint to update the current conf

Review comment:
       Yes, changes are mainly for validation through createClusterConfig method. For update, just renamed "updateConfig" to "updateClusterConfig"




----------------------------------------------------------------
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 #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (fb107bf) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.51%`.
   > The diff coverage is `83.55%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   63.98%   +0.51%     
   ==========================================
     Files          60       60              
     Lines        5220     5300      +80     
   ==========================================
   + Hits         3313     3391      +78     
   - Misses       1747     1748       +1     
   - Partials      160      161       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `6.28% <0.00%> (+0.01%)` | :arrow_up: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `48.73% <0.00%> (-0.18%)` | :arrow_down: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <66.66%> (+0.16%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `65.94% <69.23%> (-0.11%)` | :arrow_down: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.20% <84.44%> (+4.07%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.78% <100.00%> (+0.75%)` | :arrow_up: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [55d404a...fb107bf](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] holdenk commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if queryParams.Get("dry_run") != "1" {
+		http.Error(w, "Invalid query params. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}

Review comment:
       This seems to require that dry run is set, which is confusing. Could the error message maybe state what parameter is missing or what parameter is invalid?




----------------------------------------------------------------
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 #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (f90d7e7) 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 `83.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   63.95%   +0.48%     
   ==========================================
     Files          60       60              
     Lines        5220     5302      +82     
   ==========================================
   + Hits         3313     3391      +78     
   - Misses       1747     1749       +2     
   - Partials      160      162       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `6.28% <0.00%> (+0.01%)` | :arrow_up: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `48.73% <0.00%> (-0.18%)` | :arrow_down: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `65.94% <69.23%> (-0.11%)` | :arrow_down: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.20% <84.78%> (+4.07%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/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/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [4c53c50...f90d7e7](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] codecov[bot] edited a comment on pull request #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (f954680) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `1.59%`.
   > The diff coverage is `68.82%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   65.06%   +1.59%     
   ==========================================
     Files          60       60              
     Lines        5220     5536     +316     
   ==========================================
   + Hits         3313     3602     +289     
   - Misses       1747     1760      +13     
   - Partials      160      174      +14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.78% <0.00%> (-0.49%)` | :arrow_down: |
   | [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `69.18% <63.63%> (-0.27%)` | :arrow_down: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `56.41% <67.85%> (+7.50%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `72.53% <82.95%> (+6.49%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.45% <85.41%> (+4.31%)` | :arrow_up: |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [5e8696c...f954680](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] codecov[bot] edited a comment on pull request #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (fb107bf) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `0.51%`.
   > The diff coverage is `83.55%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   63.98%   +0.51%     
   ==========================================
     Files          60       60              
     Lines        5220     5300      +80     
   ==========================================
   + Hits         3313     3391      +78     
   - Misses       1747     1748       +1     
   - Partials      160      161       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `6.28% <0.00%> (+0.01%)` | :arrow_up: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `48.73% <0.00%> (-0.18%)` | :arrow_down: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <66.66%> (+0.16%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `65.94% <69.23%> (-0.11%)` | :arrow_down: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.20% <84.44%> (+4.07%)` | :arrow_up: |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `90.00% <100.00%> (+2.99%)` | :arrow_up: |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `96.78% <100.00%> (+0.75%)` | :arrow_up: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `95.77% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [4c53c50...f90d7e7](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] kingamarton commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/dao/error_info.go
##########
@@ -0,0 +1,33 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package dao
+
+type YAPIError struct {
+	StatusCode  int    `json:"status_code"`
+	Message     string `json:"message"`
+	Description string `json:"description"`
+}
+
+func NewYAPIError(err error, statusCode int, message string) *YAPIError {

Review comment:
       The `err` parameter is not used. The description should have the value of the `err error` parameter, no?

##########
File path: pkg/webservice/routes.go
##########
@@ -102,7 +102,15 @@ var webRoutes = routes{
 		"Scheduler",
 		"PUT",
 		"/ws/v1/config",
-		updateConfig,
+		updateClusterConfig,
+	},
+
+	// endpoint to update the current conf

Review comment:
       please update the comment here, since this is the endpoint for validating the config, not for updating it.




-- 
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 #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (f1b0619) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `1.59%`.
   > The diff coverage is `68.82%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   65.06%   +1.59%     
   ==========================================
     Files          60       60              
     Lines        5220     5536     +316     
   ==========================================
   + Hits         3313     3602     +289     
   - Misses       1747     1760      +13     
   - Partials      160      174      +14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.78% <0.00%> (-0.49%)` | :arrow_down: |
   | [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `69.18% <63.63%> (-0.27%)` | :arrow_down: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `56.41% <67.85%> (+7.50%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `72.53% <82.95%> (+6.49%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.45% <85.41%> (+4.31%)` | :arrow_up: |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [5e8696c...f1b0619](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] kingamarton commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

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



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)

Review comment:
       Pleas include in the error message exactly what parameter is missing. In our case "dry_run"

##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]

Review comment:
       Please don't ignore the returned value here and reuse it in the line 469:
   
   ```
   dryRun, dryRunExists := queryParams["dry_run"]
   ....
   if dryRun != "1" {
   ...
   ```

##########
File path: pkg/webservice/routes.go
##########
@@ -102,7 +102,15 @@ var webRoutes = routes{
 		"Scheduler",
 		"PUT",
 		"/ws/v1/config",
-		updateConfig,
+		updateClusterConfig,
+	},
+
+	// endpoint to update the current conf

Review comment:
       this is the endpoint to validate the config, not? the update is the previous one.

##########
File path: pkg/webservice/webservice.go
##########
@@ -58,14 +59,40 @@ func newRouter() *mux.Router {
 func loggingHandler(inner http.Handler, name string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
-
-		inner.ServeHTTP(w, r)
-
+		rw := &YResponseWriter{ResponseWriter: w, statusCode: -1}
+		inner.ServeHTTP(rw, r)
+		var body = string(rw.body)
+		if rw.statusCode != 200 && rw.statusCode != -1 {
+			errorInfo := dao.NewYAPIError(nil, rw.statusCode, body)

Review comment:
       This `NewYAPIError` method is not defined. This is the reason why Travis check is failing. 

##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if queryParams.Get("dry_run") != "1" {
+		http.Error(w, "Invalid query params. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}

Review comment:
       Right now we support only dry_run=1. I think we should document it and also include it in the error message.




----------------------------------------------------------------
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 #231: YUNIKORN-418: Add "config" REST API

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=h1) Report
   > Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=desc) (f954680) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/5a1c19e280f66d6cb4fc206a675116260d6f2ff1?el=desc) (5a1c19e) will **increase** coverage by `1.59%`.
   > The diff coverage is `68.82%`.
   
   > :exclamation: Current head f954680 differs from pull request most recent head f1b0619. Consider uploading reports for the commit f1b0619 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #231      +/-   ##
   ==========================================
   + Coverage   63.46%   65.06%   +1.59%     
   ==========================================
     Files          60       60              
     Lines        5220     5536     +316     
   ==========================================
   + Hits         3313     3602     +289     
   - Misses       1747     1760      +13     
   - Partials      160      174      +14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/context.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `5.78% <0.00%> (-0.49%)` | :arrow_down: |
   | [pkg/scheduler/nodes\_usage\_monitor.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9ub2Rlc191c2FnZV9tb25pdG9yLmdv) | `0.00% <0.00%> (ø)` | |
   | [pkg/scheduler/objects/allocation.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FsbG9jYXRpb24uZ28=) | `100.00% <ø> (ø)` | |
   | [pkg/scheduler/partition\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb25fbWFuYWdlci5nbw==) | `20.00% <50.00%> (ø)` | |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `69.18% <63.63%> (-0.27%)` | :arrow_down: |
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `56.41% <67.85%> (+7.50%)` | :arrow_up: |
   | [pkg/scheduler/objects/node.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `81.98% <75.00%> (+0.16%)` | :arrow_up: |
   | [pkg/webservice/webservice.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2Uvd2Vic2VydmljZS5nbw==) | `44.44% <75.00%> (+31.54%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `72.53% <82.95%> (+6.49%)` | :arrow_up: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `58.45% <85.41%> (+4.31%)` | :arrow_up: |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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/231?src=pr&el=footer). Last update [5e8696c...f1b0619](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/231?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] kingamarton closed pull request #231: YUNIKORN-418: Add "config" REST API

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


   


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