pbacsko commented on code in PR #928:
URL: https://github.com/apache/yunikorn-k8shim/pull/928#discussion_r1805253691
##########
pkg/cache/application.go:
##########
@@ -71,6 +71,7 @@ func (app *Application) String() string {
func NewApplication(appID, queueName, user string, groups []string, tags
map[string]string, scheduler api.SchedulerAPI) *Application {
taskMap := make(map[string]*Task)
+ taskGroups := make(map[string]*TaskGroup)
Review Comment:
Does it have to be a pointer? Probably not.
##########
pkg/cache/application.go:
##########
@@ -181,7 +189,14 @@ func (app *Application) getPlaceholderAsk() *si.Resource {
func (app *Application) getTaskGroups() []TaskGroup {
app.lock.RLock()
defer app.lock.RUnlock()
- return app.taskGroups
+
+ taskGroups := make([]TaskGroup, 0)
+ if len(app.taskGroups) > 0 {
+ for _, taskGroup := range app.taskGroups {
+ taskGroups = append(taskGroups, *taskGroup)
+ }
+ }
+ return taskGroups
Review Comment:
nit:
```
if len(app.taskGroups) > 0 {
taskGroups := make([]TaskGroup, 0, len(app.taskGroups))
for _, taskGroup := range app.taskGroups {
taskGroups := append(taskGroups , *taskGroup)
}
return taskGroups
}
return nil
```
##########
pkg/cache/application.go:
##########
@@ -166,9 +167,16 @@ func (app *Application) GetSchedulingParamsDefinition()
string {
func (app *Application) setTaskGroups(taskGroups []TaskGroup) {
app.lock.Lock()
defer app.lock.Unlock()
- app.taskGroups = taskGroups
- for _, taskGroup := range app.taskGroups {
- app.placeholderAsk = common.Add(app.placeholderAsk,
common.GetTGResource(taskGroup.MinResource, int64(taskGroup.MinMember)))
+ for _, tg := range taskGroups {
+ taskGroup := tg
+ if _, exists := app.taskGroups[taskGroup.Name]; exists {
+ log.Log(log.ShimCacheApplication).Warn("duplicate
task-group within the task-groups",
+ zap.String("appID", app.applicationID),
+ zap.String("groupName", taskGroup.Name))
Review Comment:
Make sure you have an unit test which covers this branch.
##########
pkg/cache/application.go:
##########
@@ -80,7 +81,7 @@ func NewApplication(appID, queueName, user string, groups
[]string, tags map[str
taskMap: taskMap,
tags: tags,
sm: newAppState(),
- taskGroups: make([]TaskGroup, 0),
+ taskGroups: taskGroups,
Review Comment:
nit:
```
taskGroups: make(map[string]*TaskGroup]
```
##########
pkg/cache/application.go:
##########
@@ -166,9 +167,16 @@ func (app *Application) GetSchedulingParamsDefinition()
string {
func (app *Application) setTaskGroups(taskGroups []TaskGroup) {
app.lock.Lock()
defer app.lock.Unlock()
- app.taskGroups = taskGroups
- for _, taskGroup := range app.taskGroups {
- app.placeholderAsk = common.Add(app.placeholderAsk,
common.GetTGResource(taskGroup.MinResource, int64(taskGroup.MinMember)))
+ for _, tg := range taskGroups {
+ taskGroup := tg
Review Comment:
If the map is `string[TaskGroup]` then this is not needed I believe.
--
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]
