pbacsko commented on code in PR #750:
URL: https://github.com/apache/yunikorn-core/pull/750#discussion_r1419082299
##########
pkg/webservice/handlers.go:
##########
@@ -619,6 +628,58 @@ func getQueueApplications(w http.ResponseWriter, r
*http.Request) {
}
}
+func getPartitionApplicationsByState(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ vars := httprouter.ParamsFromContext(r.Context())
+ if vars == nil {
+ buildJSONErrorResponse(w, MissingParamsName,
http.StatusBadRequest)
+ return
+ }
+ partition := vars.ByName("partition")
+ appState := vars.ByName("state")
+
+ partitionContext :=
schedulerContext.GetPartitionWithoutClusterID(partition)
+ if partitionContext == nil {
+ buildJSONErrorResponse(w, PartitionDoesNotExists,
http.StatusNotFound)
+ return
+ }
+ stateErr := validateApplicationState(appState)
Review Comment:
Define a map like:
```
map disallowedAppStates = map[string]bool{
objects.Running.String(): true,
objects.Rejected.String(): true,
objects.Completed.String(): true
}
...
if disallowedAppState[appState] {
buildJSONErrorResponse(w,"Application state is not allowed: " + appState,
http.StatusBadRequest)
return
}
```
##########
pkg/webservice/handlers.go:
##########
@@ -88,6 +88,15 @@ func getClusterInfo(w http.ResponseWriter, r *http.Request) {
}
}
+func validateApplicationState(appType string) error {
Review Comment:
"appState" instead of "appType", but I don't think this tiny helper is
necessary, see below
##########
pkg/scheduler/partition.go:
##########
@@ -425,6 +425,10 @@ func (pc *PartitionContext) removeAppInternal(appID
string) *objects.Application
return app
}
+func (pc *PartitionContext) GetApplication(appID string) *objects.Application {
+ return pc.getApplication(appID)
+}
Review Comment:
RLock/RUnlock
##########
pkg/scheduler/partition.go:
##########
@@ -1066,6 +1070,18 @@ func (pc *PartitionContext) GetRejectedApplications()
[]*objects.Application {
return appList
}
+func (pc *PartitionContext) GetAppsByState(state string) []string {
Review Comment:
1. RLock/RUnlock
2. We already have a `getAppsByState()` which doesn't do any kind of
filtering. Need to an a comment to explain why this method does what it does.
Even a different name is should be considered.
In Yunikorn, when we have `xMethodName()` and `XMethodName()` methods, the
exported one performs locking/unlocking and calls the unexported one. This
methods completely breaks this convention.
##########
pkg/webservice/handlers.go:
##########
@@ -619,6 +628,58 @@ func getQueueApplications(w http.ResponseWriter, r
*http.Request) {
}
}
+func getPartitionApplicationsByState(w http.ResponseWriter, r *http.Request) {
Review Comment:
Function name: you're not returning applications but application IDs. A
better name is `getPartitionApplicationIDsByState()`.
##########
pkg/webservice/handlers.go:
##########
@@ -619,6 +628,58 @@ func getQueueApplications(w http.ResponseWriter, r
*http.Request) {
}
}
+func getPartitionApplicationsByState(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ vars := httprouter.ParamsFromContext(r.Context())
+ if vars == nil {
+ buildJSONErrorResponse(w, MissingParamsName,
http.StatusBadRequest)
+ return
+ }
+ partition := vars.ByName("partition")
+ appState := vars.ByName("state")
+
+ partitionContext :=
schedulerContext.GetPartitionWithoutClusterID(partition)
+ if partitionContext == nil {
+ buildJSONErrorResponse(w, PartitionDoesNotExists,
http.StatusNotFound)
+ return
+ }
+ stateErr := validateApplicationState(appState)
+ if stateErr != nil {
+ buildJSONErrorResponse(w, stateErr.Error(),
http.StatusBadRequest)
+ return
+ }
+ appIDs := partitionContext.GetAppsByState(appState)
+ appIDsDao := dao.ApplicationIDsDAOInfo{AppIDs: appIDs}
+ if err := json.NewEncoder(w).Encode(appIDsDao); err != nil {
+ buildJSONErrorResponse(w, err.Error(),
http.StatusInternalServerError)
+ }
+}
+
+func getPartitionApplication(w http.ResponseWriter, r *http.Request) {
+ writeHeaders(w)
+ vars := httprouter.ParamsFromContext(r.Context())
+ if vars == nil {
+ buildJSONErrorResponse(w, MissingParamsName,
http.StatusBadRequest)
+ return
+ }
+ partition := vars.ByName("partition")
+ application := vars.ByName("application")
Review Comment:
Check valid partition and application (`partition != ""` and `application !=
""`)
##########
pkg/scheduler/partition.go:
##########
@@ -1066,6 +1070,18 @@ func (pc *PartitionContext) GetRejectedApplications()
[]*objects.Application {
return appList
}
+func (pc *PartitionContext) GetAppsByState(state string) []string {
Review Comment:
In fact, we can do better... These changes can be done in a follow-up JIRA,
not in this one.
Right now, this is what have:
`type applicationState int`
However I think it's better to have it as:
```
type ApplicationState string
const (
New ApplicationState = "New"
Running = "Running"
...
Unknown = "Unknown"
)
```
Put the available strings to a map:
```
var validAppStates = map[string]ApplicationState{
"Running": Running,
"New": New,
...
}
```
When you parse the incoming value, you can create a `fromString()` function:
```
func fromString(appState string) ApplicationState {
state, ok := validAppStates[appState]; ok {
return state
}
return Unknown
}
```
##########
pkg/webservice/handlers_test.go:
##########
@@ -1088,8 +1088,13 @@ func addAppWithUserGroup(t *testing.T, id string, part
*scheduler.PartitionConte
assert.Equal(t, 1+initSize, len(part.GetApplications()))
if isCompleted {
app.SetState(objects.Completing.String())
+ currentCount := len(part.GetCompletedApplications())
err = app.HandleApplicationEvent(objects.CompleteApplication)
assert.NilError(t, err, "The app should have completed")
+ err = common.WaitFor(10*time.Millisecond,
time.Duration(1000)*time.Millisecond, func() bool {
Review Comment:
`time.Duration(1000)*time.Millisecond` --> `time.Second()`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
