wilfred-s commented on a change in pull request #243:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/243#discussion_r598467785
##########
File path: pkg/common/constants/constants.go
##########
@@ -55,7 +55,7 @@ const AppManagerHandlerName = "yunikorn-app"
const PlaceholderContainerImage = "k8s.gcr.io/pause"
const PlaceholderContainerName = "pause"
const PlaceholderPodRestartPolicy = "Never"
-const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
+const PlaceholderFlag = "yunikorn.apache.org/placeholder"
Review comment:
For the `ApplicationID` we use an unqualified `applicationId` in labels
and a qualified version `yunikorn.apache.org/app-id` in the annotation.
Shouldn't we follow the same rule here? I can live with either solution, I just
think we need to set a standard for what we do.
##########
File path: pkg/common/utils/gang_utils.go
##########
@@ -75,7 +75,7 @@ func GetPlaceholderResourceRequest(resources
map[string]resource.Quantity) v1.Re
}
func GetPlaceholderFlagFromPodSpec(pod *v1.Pod) bool {
- if value, ok := pod.Annotations[constants.AnnotationPlaceholderFlag];
ok {
+ if value, ok := pod.Annotations[constants.PlaceholderFlag]; ok {
Review comment:
Do we add the label just as a convenience or is there more behind this?
The appID retrieval uses a logic to look at annotations and then labels
(normal followed by spark).
--
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:
[email protected]
