You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@yunikorn.apache.org by GitBox <gi...@apache.org> on 2020/03/17 04:04:52 UTC
[GitHub] [incubator-yunikorn-core] TaoYang526 opened a new pull request
#103: [YUNIKORN-28] Support validating conf via REST API
TaoYang526 opened a new pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103
As discussed in https://github.com/apache/incubator-yunikorn-k8shim/pull/81, we need to validate conf via REST API in scheduler core.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393505986
##########
File path: pkg/webservice/handlers.go
##########
@@ -118,6 +120,24 @@ func GetNodesInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func ValidateConf(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err == nil {
+ _, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ }
+ var result dao.ValidateConfResponse
+ if err != nil {
+ result.Allowed = false
+ result.Error = err.Error()
+ } else {
+ result.Allowed = true
+ }
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ panic(err)
Review comment:
@yangwwei You are right, http.Error is the better way to handle this. I have tested panic and http.Error: (1) When panic happened, the scheduler process didn't stop (the connection is running in a new go-routine and I think the panic must be handled), and client got error like this: `curl: (52) Empty reply from server`; (2) When I replaced it with http.Error like this: `http.Error(w, err.Error(), http.StatusInternalServerError)`, client can got the error message in the body of response but status code is 200, cased by we have updated the status code before handling this request in handlers.go#writeHeaders, it can be fixed when I removed the last line: `w.WriteHeader(http.StatusOK)`.
Now all related places have been corrected, please take a look again. Thanks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393454189
##########
File path: pkg/webservice/handlers.go
##########
@@ -118,6 +120,24 @@ func GetNodesInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func ValidateConf(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err == nil {
+ _, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ }
+ var result dao.ValidateConfResponse
+ if err != nil {
+ result.Allowed = false
+ result.Error = err.Error()
+ } else {
+ result.Allowed = true
+ }
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ panic(err)
Review comment:
Sorry I was not quite clear. I know, this is a historic issue, not a new issue with this PR.
We should not have any panic in our code, if something really goes wrong, we can send the error via `http.Error()`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393438386
##########
File path: pkg/webservice/handlers.go
##########
@@ -118,6 +120,24 @@ func GetNodesInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func ValidateConf(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err == nil {
+ _, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ }
+ var result dao.ValidateConfResponse
+ if err != nil {
+ result.Allowed = false
+ result.Error = err.Error()
+ } else {
+ result.Allowed = true
+ }
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ panic(err)
Review comment:
Hmmm, we should not have any `panic` here.
Can we replace all panic to `http.Error()`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393545895
##########
File path: pkg/webservice/handlers.go
##########
@@ -118,6 +120,24 @@ func GetNodesInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func ValidateConf(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err == nil {
+ _, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ }
+ var result dao.ValidateConfResponse
+ if err != nil {
+ result.Allowed = false
+ result.Error = err.Error()
+ } else {
+ result.Allowed = true
+ }
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ panic(err)
Review comment:
Oh, that's a bug. Thanks for finding this out.
The fix looks all good to me. Approving the PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393444050
##########
File path: pkg/webservice/dao/validate_info.go
##########
@@ -0,0 +1,24 @@
+/*
+ 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 ValidateConfResponse struct {
+ Allowed bool
+ Error string
Review comment:
make sense. Thanks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393444706
##########
File path: pkg/webservice/handlers.go
##########
@@ -118,6 +120,24 @@ func GetNodesInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func ValidateConf(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err == nil {
+ _, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ }
+ var result dao.ValidateConfResponse
+ if err != nil {
+ result.Allowed = false
+ result.Error = err.Error()
+ } else {
+ result.Allowed = true
+ }
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ panic(err)
Review comment:
Here is just like other handers, I'm not sure if it is intentional, may be panic can be caught by the framework of rest server?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on issue #103:
[YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
yangwwei commented on issue #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#issuecomment-600878649
LGTM, merging this 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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393444706
##########
File path: pkg/webservice/handlers.go
##########
@@ -118,6 +120,24 @@ func GetNodesInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func ValidateConf(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ requestBytes, err := ioutil.ReadAll(r.Body)
+ if err == nil {
+ _, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ }
+ var result dao.ValidateConfResponse
+ if err != nil {
+ result.Allowed = false
+ result.Error = err.Error()
+ } else {
+ result.Allowed = true
+ }
+ if err := json.NewEncoder(w).Encode(result); err != nil {
+ panic(err)
Review comment:
Here is just like other handers, I'm not sure is it intentional, may be panic can be caught by the framework of rest server?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei merged pull request #103:
[YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull
request #103: [YUNIKORN-28] Support validating conf via REST API
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #103: [YUNIKORN-28] Support validating conf via REST API
URL: https://github.com/apache/incubator-yunikorn-core/pull/103#discussion_r393436657
##########
File path: pkg/webservice/dao/validate_info.go
##########
@@ -0,0 +1,24 @@
+/*
+ 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 ValidateConfResponse struct {
+ Allowed bool
+ Error string
Review comment:
Rename `Error` to `Reason`, error usually means an error type, reason can be a string.
Can we also add the annotations: e.g `json:"allowed"`, for serialization/deserilization
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: dev-help@yunikorn.apache.org