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 2021/04/14 23:37:32 UTC
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator
yangwwei commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613648898
##########
File path: deployments/image/configmap/Dockerfile
##########
@@ -46,8 +46,8 @@ ENV DISPATCHER_TIMEOUT "300s"
ENV KUBE_CLIENT_QPS "1000"
ENV KUBE_CLIENT_BURST "1000"
ENV PREDICATES ""
-ENV OPERATOR_PLUGINS "general"
ENV ENABLE_CONFIG_HOT_REFRESH "true"
+ENV OPERATOR_PLUGINS "general,spark-k8s-operator"
Review comment:
this change will build the docker image by default with spark-k8s-operator plugin enabled.
We might want to provide an option instead of doing this by default. I think we can add the env var `OPERATOR_PLUGINS` in the helm chart template: https://github.com/apache/incubator-yunikorn-release/blob/2ade62ac43f9f62f804739f418fcdcb256b9703b/helm-charts/yunikorn/templates/deployment.yaml#L55. And expose this in https://github.com/apache/incubator-yunikorn-release/blob/master/helm-charts/yunikorn/values.yaml.
##########
File path: go.mod
##########
@@ -21,7 +21,7 @@ module github.com/apache/incubator-yunikorn-k8shim
go 1.12
require (
- github.com/GoogleCloudPlatform/spark-on-k8s-operator v0.0.0-20200817155620-c19d2b8660d8
+ github.com/GoogleCloudPlatform/spark-on-k8s-operator v0.0.0-20201215015655-2e8b733f5ad0
Review comment:
which version of spark-k8s-operator this line point to?
I think we need to point to the most stable released version
##########
File path: pkg/cache/application.go
##########
@@ -460,9 +461,14 @@ func (app *Application) postAppAccepted() {
// app could have allocated tasks upon a recovery, and in that case,
// the reserving phase has already passed, no need to trigger that again.
var ev events.SchedulingEvent
+ log.Logger().Info("postAppAccepted on cached app",
+ zap.String("appID", app.applicationID),
+ zap.Int("numTaskGroups", len(app.taskGroups)),
+ zap.Int("numAllocatedTasks", len(app.getTasks(events.States().Task.Allocated))))
if len(app.taskGroups) != 0 &&
len(app.getTasks(events.States().Task.Allocated)) == 0 {
ev = NewSimpleApplicationEvent(app.applicationID, events.TryReserve)
+ log.Logger().Info("tryReserve")
Review comment:
minor: pls improve this line of logging
something like: "app <appID> has taskGroups defined, trying to reserve resources for the gang members"
##########
File path: pkg/cache/application.go
##########
@@ -460,9 +461,14 @@ func (app *Application) postAppAccepted() {
// app could have allocated tasks upon a recovery, and in that case,
// the reserving phase has already passed, no need to trigger that again.
var ev events.SchedulingEvent
+ log.Logger().Info("postAppAccepted on cached app",
+ zap.String("appID", app.applicationID),
+ zap.Int("numTaskGroups", len(app.taskGroups)),
+ zap.Int("numAllocatedTasks", len(app.getTasks(events.States().Task.Allocated))))
Review comment:
this seems more like a DEBUG level logging
any reason why we want to expose this in INFO level?
##########
File path: pkg/appmgmt/sparkoperator/spark.go
##########
@@ -23,25 +23,14 @@ import (
crcClientSet "github.com/GoogleCloudPlatform/spark-on-k8s-operator/pkg/client/clientset/versioned"
crInformers "github.com/GoogleCloudPlatform/spark-on-k8s-operator/pkg/client/informers/externalversions"
"go.uber.org/zap"
- "k8s.io/api/core/v1"
- "k8s.io/apimachinery/pkg/labels"
k8sCache "k8s.io/client-go/tools/cache"
"github.com/apache/incubator-yunikorn-k8shim/pkg/appmgmt/interfaces"
"github.com/apache/incubator-yunikorn-k8shim/pkg/client"
- "github.com/apache/incubator-yunikorn-k8shim/pkg/common"
- "github.com/apache/incubator-yunikorn-k8shim/pkg/common/constants"
- "github.com/apache/incubator-yunikorn-k8shim/pkg/common/utils"
"github.com/apache/incubator-yunikorn-k8shim/pkg/log"
- "github.com/apache/incubator-yunikorn-scheduler-interface/lib/go/si"
)
-const (
- LabelAnnotationPrefix = "sparkoperator.k8s.io/"
- SparkAppNameLabel = LabelAnnotationPrefix + "app-name"
-)
-
-// implements interfaces#Recoverable, interfaces#AppManager
+// Manager implements interfaces#Recoverable, interfaces#AppManager
Review comment:
minor: suggest to add some more code comment to explain what has been done by this plugin
##########
File path: pkg/common/events/events.go
##########
@@ -33,20 +33,20 @@ type SchedulingEvent interface {
type ApplicationEventType string
const (
- SubmitApplication ApplicationEventType = "SubmitApplication"
- RecoverApplication ApplicationEventType = "RecoverApplication"
- AcceptApplication ApplicationEventType = "AcceptApplication"
- TryReserve ApplicationEventType = "TryReserve"
- UpdateReservation ApplicationEventType = "UpdateReservation"
- RunApplication ApplicationEventType = "RunApplication"
- RejectApplication ApplicationEventType = "RejectApplication"
- CompleteApplication ApplicationEventType = "CompleteApplication"
- FailApplication ApplicationEventType = "FailApplication"
- KillApplication ApplicationEventType = "KillApplication"
- KilledApplication ApplicationEventType = "KilledApplication"
- ReleaseAppAllocation ApplicationEventType = "ReleaseAppAllocation"
+ SubmitApplication ApplicationEventType = "SubmitApplication"
+ RecoverApplication ApplicationEventType = "RecoverApplication"
+ AcceptApplication ApplicationEventType = "AcceptApplication"
+ TryReserve ApplicationEventType = "TryReserve"
+ UpdateReservation ApplicationEventType = "UpdateReservation"
+ RunApplication ApplicationEventType = "RunApplication"
+ RejectApplication ApplicationEventType = "RejectApplication"
+ CompleteApplication ApplicationEventType = "CompleteApplication"
+ FailApplication ApplicationEventType = "FailApplication"
+ KillApplication ApplicationEventType = "KillApplication"
+ KilledApplication ApplicationEventType = "KilledApplication"
+ ReleaseAppAllocation ApplicationEventType = "ReleaseAppAllocation"
ReleaseAppAllocationAsk ApplicationEventType = "ReleaseAppAllocationAsk"
- AppStateChange ApplicationEventType = "ApplicationStateChange"
+ AppStateChange ApplicationEventType = "ApplicationStateChange"
Review comment:
do we have any change here other than formating?
if not, prefer not to change this in this 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