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 2022/08/09 10:33:32 UTC
[GitHub] [yunikorn-core] manirajv06 opened a new pull request, #428: [YUNIKORN-1276] REST API for specific application
manirajv06 opened a new pull request, #428:
URL: https://github.com/apache/yunikorn-core/pull/428
### What is this PR for?
Exposed an REST API to fetch the specific application details.
### What type of PR is it?
* [ ] - Improvement
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1276
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #428: [YUNIKORN-1276] REST API for specific application
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #428:
URL: https://github.com/apache/yunikorn-core/pull/428#issuecomment-1209211210
# [Codecov](https://codecov.io/gh/apache/yunikorn-core/pull/428?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#428](https://codecov.io/gh/apache/yunikorn-core/pull/428?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (623ebf5) into [master](https://codecov.io/gh/apache/yunikorn-core/commit/d74e7283b38eb0ce3645e1502e06735dbb202e3e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d74e728) will **decrease** coverage by `0.10%`.
> The diff coverage is `48.93%`.
```diff
@@ Coverage Diff @@
## master #428 +/- ##
==========================================
- Coverage 71.87% 71.76% -0.11%
==========================================
Files 65 65
Lines 9602 9647 +45
==========================================
+ Hits 6901 6923 +22
- Misses 2458 2475 +17
- Partials 243 249 +6
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-core/pull/428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/webservice/handlers.go](https://codecov.io/gh/apache/yunikorn-core/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `68.15% <48.88%> (-1.25%)` | :arrow_down: |
| [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/yunikorn-core/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `69.90% <50.00%> (ø)` | |
:mega: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] manirajv06 commented on a diff in pull request #428: [YUNIKORN-1276] REST API for specific application
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on code in PR #428:
URL: https://github.com/apache/yunikorn-core/pull/428#discussion_r943158875
##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
}
+func getApplication(w http.ResponseWriter, r *http.Request) {
+ vars := mux.Vars(r)
+ writeHeaders(w)
+ partition, partitionExists := vars["partition"]
+ if !partitionExists {
+ buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+ queueName, queueNameExists := vars["queue"]
+ if !queueNameExists {
+ buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ application, applicationExists := vars["application"]
+ if !applicationExists {
+ buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ queueErr := validateQueue(queueName)
+ if queueErr != nil {
+ buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+ return
+ }
+ if len(vars) != 3 {
+ buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
Review Comment:
We followed the same order while redesigned the whole REST API's. But then realised that doing 2 before 1 gives clear and concise err message by doing simple check on request parameters. For example, Pointing out specific param is missing is very clear instead of saying generic incorrect url path or something like that. We had discussion around this as part of design and through jira comments as well. Finally, we landed up in the current order. If you still think, may be we can handle this new jira as this require changes in all API's.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #428: [YUNIKORN-1276] REST API for specific application
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #428:
URL: https://github.com/apache/yunikorn-core/pull/428#discussion_r944064005
##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
}
+func getApplication(w http.ResponseWriter, r *http.Request) {
+ vars := mux.Vars(r)
+ writeHeaders(w)
+ partition, partitionExists := vars["partition"]
+ if !partitionExists {
+ buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+ queueName, queueNameExists := vars["queue"]
+ if !queueNameExists {
+ buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ application, applicationExists := vars["application"]
+ if !applicationExists {
+ buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ queueErr := validateQueue(queueName)
+ if queueErr != nil {
+ buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+ return
+ }
+ if len(vars) != 3 {
+ buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
Review Comment:
Dug in a bit further: the URL matching inside gorilla already filters out anything malformed. The definition of the route makes sure that we always get the values. As per the [examples](https://github.com/gorilla/mux#examples) in the docs:
```
Paths can have variables. They are defined using the format {name} or {name:pattern}. If a regular expression pattern is not defined, the matched variable will be anything until the next slash.
```
The mux code does a regexp on the URL to see if it matches the route. If that regexp does not match a 404 is returned. I have tried to craft a request that will call the handler which does not have the parameters set and I cannot. When I get to the point that the handler is called I have the values in the map. The content might be garbage but I have content. I was not able to match the route in any other way.
In conclusion: anything that does not map to a route will give back a 404. Check 1 and 2 are thus completely unneeded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] manirajv06 commented on a diff in pull request #428: [YUNIKORN-1276] REST API for specific application
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on code in PR #428:
URL: https://github.com/apache/yunikorn-core/pull/428#discussion_r943158875
##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
}
+func getApplication(w http.ResponseWriter, r *http.Request) {
+ vars := mux.Vars(r)
+ writeHeaders(w)
+ partition, partitionExists := vars["partition"]
+ if !partitionExists {
+ buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+ queueName, queueNameExists := vars["queue"]
+ if !queueNameExists {
+ buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ application, applicationExists := vars["application"]
+ if !applicationExists {
+ buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ queueErr := validateQueue(queueName)
+ if queueErr != nil {
+ buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+ return
+ }
+ if len(vars) != 3 {
+ buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
Review Comment:
We followed the same order while redesigned the whole REST API's. But then realised that doing 2 before 1 gives clear and concise err message by doing simple check on request parameters. For example, Pointing out specific param is missing is very clear instead of saying generic incorrect url path or something like that. We had discussion around this as part of design and through jira comments as well. Finally, we landed up in the current order. If you still think, may be we can handle this in new jira as this require changes in all API's.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #428: [YUNIKORN-1276] REST API for specific application
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #428:
URL: https://github.com/apache/yunikorn-core/pull/428#discussion_r942162271
##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
}
+func getApplication(w http.ResponseWriter, r *http.Request) {
+ vars := mux.Vars(r)
+ writeHeaders(w)
+ partition, partitionExists := vars["partition"]
+ if !partitionExists {
+ buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+ queueName, queueNameExists := vars["queue"]
+ if !queueNameExists {
+ buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ application, applicationExists := vars["application"]
+ if !applicationExists {
+ buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ queueErr := validateQueue(queueName)
+ if queueErr != nil {
+ buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+ return
+ }
+ if len(vars) != 3 {
+ buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
Review Comment:
We need to be consistent with these checks. I would expect a standard ordering:
1. the number of arguments
2. the existence of each argument by name
3. the content of the argument syntax check (if required)
We do not seem to do that consistently in any of the calls. It means we do too much processing before the eventual failure.
##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
}
+func getApplication(w http.ResponseWriter, r *http.Request) {
+ vars := mux.Vars(r)
+ writeHeaders(w)
+ partition, partitionExists := vars["partition"]
+ if !partitionExists {
+ buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
Review Comment:
Need to make this constants or something not add the same text in multiple places in the handler, same for all error messages for parameter checks.
##########
pkg/webservice/handlers.go:
##########
@@ -677,6 +677,62 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
}
+func getApplication(w http.ResponseWriter, r *http.Request) {
+ vars := mux.Vars(r)
+ writeHeaders(w)
+ partition, partitionExists := vars["partition"]
+ if !partitionExists {
+ buildJSONErrorResponse(w, "Partition is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+ queueName, queueNameExists := vars["queue"]
+ if !queueNameExists {
+ buildJSONErrorResponse(w, "Queue is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ application, applicationExists := vars["application"]
+ if !applicationExists {
+ buildJSONErrorResponse(w, "Application Id is missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ queueErr := validateQueue(queueName)
+ if queueErr != nil {
+ buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
+ return
+ }
+ if len(vars) != 3 {
+ buildJSONErrorResponse(w, "Incorrect URL path. Please check the usage documentation", http.StatusBadRequest)
+ return
+ }
+
+ partitionContext := schedulerContext.GetPartitionWithoutClusterID(partition)
+ if partitionContext == nil {
+ buildJSONErrorResponse(w, "Partition not found", http.StatusBadRequest)
+ return
+ }
+
+ queue := partitionContext.GetQueue(queueName)
+ if queue == nil {
+ buildJSONErrorResponse(w, "Queue not found", http.StatusBadRequest)
+ return
+ }
+
+ app := queue.GetApplication(application)
+ if app == nil {
+ buildJSONErrorResponse(w, "Application not found", http.StatusBadRequest)
+ return
+ }
+
+ appDao := make([]*dao.ApplicationDAOInfo, 0, 1)
Review Comment:
Do we really want to return an array of objects? We know we only have 1 application that we're returning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s closed pull request #428: [YUNIKORN-1276] REST API for specific application
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #428: [YUNIKORN-1276] REST API for specific application
URL: https://github.com/apache/yunikorn-core/pull/428
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org