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 01:13:03 UTC

[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 opened a new pull request #253: [YUNIKORN-558] Integration with Spark operator plugin

yuchaoran2011 opened a new pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253


   ### What is this PR for?
   Improved the integration with Spark operator plugin. The areas of improvement as as follows:
   * Make sure that individual pods fall under the responsibility of the general plugin. Make the Spark operator plugin manage the SparkApplication CRD only
   * Make sure that the general and operator plugins see the same application ID for the same SparkApplication. The ID is the string generated by the Spark K8s backend that starts with "spark-"
   * Make sure that when a SparkApplication completes or fails, cleanup is performed for all placeholders if there's any that remain at that time
   * When cleaning up placeholders, there are often cases where the placeholder doesn't exist anymore and that will cause the deletion to fail. Do not add the placeholder to the orphan pod list in this case because there's no need to retry deletion.
   * Added and updated some logging to aid monitoring and debugging
   
   ### What type of PR is it?
   * [ ] - Bug Fix
   * [ ] - Improvement
   * [ ] - Feature
   * [ ] - Documentation
   * [ ] - Hot Fix
   * [ ] - Refactoring
   
   ### Todos
   * This is only part of the work for the referenced JIRA. A separate PR will be made on https://github.com/GoogleCloudPlatform/spark-on-k8s-operator to complete the integration.
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/YUNIKORN-558 (Integrate YuniKorn with Spark K8S Operator)
   
   ### How should this be tested?
   All the existing unit tests passed. Not quite sure how to unit test this feature specifically. But probably we can enhance the integration tests to cover it. We have been running this patch for relatively large Spark operator workloads for a few weeks and it has been stable for us.
   


-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613310459



##########
File path: pkg/cache/placeholder_manager.go
##########
@@ -46,10 +47,13 @@ type PlaceholderManager struct {
 }
 
 var placeholderMgr *PlaceholderManager
+var placeholderManagerLock sync.Mutex

Review comment:
       You are exactly correct. I have fixed the `TestFailApplication` following the example of `TestTryReserve`




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613668645



##########
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:
       Sure, I'll roll back this line to include only the general plugin by default. I'll create another JIRA to track the work on exposing it in the Helm chart




-- 
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0905d7) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.45%`.
   > The diff coverage is `61.13%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.20%   +0.45%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3302     +169     
   ==========================================
   + Hits         1872     1988     +116     
   - Misses       1180     1233      +53     
     Partials       81       81              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...d0905d7](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-643] Improve the spark-k8s-operator app mgmt plugin to better handling Spark CRDs

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7b814eb) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.41%`.
   > The diff coverage is `61.29%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.17%   +0.41%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1982     +110     
   - Misses       1180     1232      +52     
   + Partials       81       80       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...7b814eb](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7b814eb) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.41%`.
   > The diff coverage is `61.29%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.17%   +0.41%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1982     +110     
   - Misses       1180     1232      +52     
   + Partials       81       80       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...7b814eb](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819949571


   It's strange why the latest commit caused e2e test failures. It only contained changes to the comments and formatting, as well as roll back the plugins that are enabled by default to the "general" only


-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613678388



##########
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:
       Ok. I'll roll back this change. It was introduced when I ran `go fmt`




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613311340



##########
File path: go.mod
##########
@@ -18,20 +18,20 @@
 
 module github.com/apache/incubator-yunikorn-k8shim
 
-go 1.12
+go 1.14

Review comment:
       Agreed. Rolled back




-- 
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-643] Improve the spark-k8s-operator app mgmt plugin to better handling Spark CRDs

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7b814eb) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.41%`.
   > The diff coverage is `61.29%`.
   
   > :exclamation: Current head 7b814eb differs from pull request most recent head 554dc46. Consider uploading reports for the commit 554dc46 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.17%   +0.41%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1982     +110     
   - Misses       1180     1232      +52     
   + Partials       81       80       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...554dc46](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613312078



##########
File path: pkg/appmgmt/general/general.go
##########
@@ -34,6 +33,8 @@ import (
 	"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"
+
+	"go.uber.org/zap"

Review comment:
       This is due to the usage of logging in this file. If I removed this import, `make lint` would complain




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613312467



##########
File path: go.mod
##########
@@ -18,20 +18,20 @@
 
 module github.com/apache/incubator-yunikorn-k8shim
 
-go 1.12
+go 1.14
 
 require (
-	github.com/GoogleCloudPlatform/spark-on-k8s-operator v0.0.0-20200817155620-c19d2b8660d8
-	github.com/apache/incubator-yunikorn-core v0.0.0-20210308173435-79a60272f12f
-	github.com/apache/incubator-yunikorn-scheduler-interface v0.9.1-0.20210226143918-19a5cca5e428
+	github.com/GoogleCloudPlatform/spark-on-k8s-operator v0.0.0-20201215015655-2e8b733f5ad0
+	github.com/apache/incubator-yunikorn-core v0.0.0-20210412120202-73fdc11674a5
+	github.com/apache/incubator-yunikorn-scheduler-interface v0.9.1-0.20210412034924-44e8abeff79e
 	github.com/google/uuid v1.1.1
 	github.com/gorilla/mux v1.7.3
 	github.com/looplab/fsm v0.1.0
-	github.com/onsi/ginkgo v1.11.0
-	github.com/onsi/gomega v1.7.0
-	go.uber.org/zap v1.13.0
+	github.com/onsi/ginkgo v1.15.1
+	github.com/onsi/gomega v1.11.0
+	go.uber.org/zap v1.16.0
 	golang.org/x/time v0.0.0-20190308202827-9d24e82272b4
-	gopkg.in/yaml.v2 v2.2.8
+	gopkg.in/yaml.v2 v2.4.0

Review comment:
       Agreed. Rolled back




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 edited a comment on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819174253


   The data race issue that happened in the unit tests has been fixed. Now ready for review


-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613683141



##########
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:
       Created https://issues.apache.org/jira/browse/YUNIKORN-642 to track the follow-up work




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613668061



##########
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:
       It's this version: https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/releases/tag/v1beta2-1.2.2-3.0.0. When I manually entered `v1beta2-1.2.2-3.0.0`, the go module system automatically changed it to the string in the 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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613316554



##########
File path: pkg/appmgmt/sparkoperator/spark.go
##########
@@ -51,6 +40,7 @@ type Manager struct {
 }
 
 func NewManager(amProtocol interfaces.ApplicationManagementProtocol, apiProvider client.APIProvider) *Manager {
+	log.Logger().Info("New AppMgr initialized")

Review comment:
       Yeah this is redundant. Removed




-- 
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-643] Improve the spark-k8s-operator app mgmt plugin to better handling Spark CRDs

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (554dc46) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.11%`.
   > The diff coverage is `61.29%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   59.86%   +0.11%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1972     +100     
   - Misses       1180     1241      +61     
     Partials       81       81              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...554dc46](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #253: [YUNIKORN-643] Improve the spark-k8s-operator app mgmt plugin to better handling Spark CRDs

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-820909726


   > It's strange why the latest commit caused e2e test failures. It only contained changes to the comments and formatting, as well as roll back the plugins that are enabled by default to the "general" only
   
   Looks like some intermittent issue, I have rerun the tests and it looks good now.


-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613313168



##########
File path: .gitignore
##########
@@ -1,3 +1,4 @@
 .idea
 _output/
+vendor

Review comment:
       Yeah this is mostly for my own convenience when looking at the code of the dependencies. Removed




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819174253


   Looks like there's a potential data race that happened in one of the unit tests. I'll look into it


-- 
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



[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #253: [YUNIKORN-643] Improve the spark-k8s-operator app mgmt plugin to better handling Spark CRDs

Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253


   


-- 
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



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613668896



##########
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:
       Yeah makes sense. I'll update to DEBUG level




-- 
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0905d7) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.45%`.
   > The diff coverage is `61.13%`.
   
   > :exclamation: Current head d0905d7 differs from pull request most recent head 39090bd. Consider uploading reports for the commit 39090bd to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.20%   +0.45%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3302     +169     
   ==========================================
   + Hits         1872     1988     +116     
   - Misses       1180     1233      +53     
     Partials       81       81              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...39090bd](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613313358



##########
File path: deployments/image/configmap/Dockerfile
##########
@@ -21,7 +21,7 @@ FROM alpine:latest
 RUN apk add curl
 RUN apk add jq
 RUN apk add --update openssl
-RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/v1.15.12/bin/linux/amd64/kubectl
+RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/v1.18.16/bin/linux/amd64/kubectl

Review comment:
       Sure. Rolled back




-- 
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-643] Improve the spark-k8s-operator app mgmt plugin to better handling Spark CRDs

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (554dc46) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.41%`.
   > The diff coverage is `61.29%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.17%   +0.41%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1982     +110     
   - Misses       1180     1232      +52     
   + Partials       81       80       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...554dc46](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d0905d7) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.45%`.
   > The diff coverage is `61.13%`.
   
   > :exclamation: Current head d0905d7 differs from pull request most recent head 105b043. Consider uploading reports for the commit 105b043 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   60.20%   +0.45%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3302     +169     
   ==========================================
   + Hits         1872     1988     +116     
   - Misses       1180     1233      +53     
     Partials       81       81              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...105b043](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] yuchaoran2011 commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
yuchaoran2011 commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r613679694



##########
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:
       I added more comments in this file to explain what the plugin does




-- 
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



[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (105b043) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.11%`.
   > The diff coverage is `61.29%`.
   
   > :exclamation: Current head 105b043 differs from pull request most recent head 7b814eb. Consider uploading reports for the commit 7b814eb to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   59.86%   +0.11%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1972     +100     
   - Misses       1180     1241      +61     
     Partials       81       81              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...7b814eb](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#discussion_r612945681



##########
File path: go.mod
##########
@@ -18,20 +18,20 @@
 
 module github.com/apache/incubator-yunikorn-k8shim
 
-go 1.12
+go 1.14
 
 require (
-	github.com/GoogleCloudPlatform/spark-on-k8s-operator v0.0.0-20200817155620-c19d2b8660d8
-	github.com/apache/incubator-yunikorn-core v0.0.0-20210308173435-79a60272f12f
-	github.com/apache/incubator-yunikorn-scheduler-interface v0.9.1-0.20210226143918-19a5cca5e428
+	github.com/GoogleCloudPlatform/spark-on-k8s-operator v0.0.0-20201215015655-2e8b733f5ad0
+	github.com/apache/incubator-yunikorn-core v0.0.0-20210412120202-73fdc11674a5
+	github.com/apache/incubator-yunikorn-scheduler-interface v0.9.1-0.20210412034924-44e8abeff79e
 	github.com/google/uuid v1.1.1
 	github.com/gorilla/mux v1.7.3
 	github.com/looplab/fsm v0.1.0
-	github.com/onsi/ginkgo v1.11.0
-	github.com/onsi/gomega v1.7.0
-	go.uber.org/zap v1.13.0
+	github.com/onsi/ginkgo v1.15.1
+	github.com/onsi/gomega v1.11.0
+	go.uber.org/zap v1.16.0
 	golang.org/x/time v0.0.0-20190308202827-9d24e82272b4
-	gopkg.in/yaml.v2 v2.2.8
+	gopkg.in/yaml.v2 v2.4.0

Review comment:
       We should not just move dependencies unless we require these changes to be made for the code to work.
   I can understand the spark operator update but we should leave the rest in this change

##########
File path: deployments/image/configmap/Dockerfile
##########
@@ -21,7 +21,7 @@ FROM alpine:latest
 RUN apk add curl
 RUN apk add jq
 RUN apk add --update openssl
-RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/v1.15.12/bin/linux/amd64/kubectl
+RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/v1.18.16/bin/linux/amd64/kubectl

Review comment:
       We need to be careful with this change. It might break 1.15 support which we have not removed yet.
   Please roll back.

##########
File path: go.mod
##########
@@ -18,20 +18,20 @@
 
 module github.com/apache/incubator-yunikorn-k8shim
 
-go 1.12
+go 1.14

Review comment:
       Changing to `go 1.14` means we require at least that version to compile. The makefile checks this. We need to announce that before we push this and make sure we have fully tested it.
   Moving the version has side effects, see my comments in [YUNIKORN-397](https://issues.apache.org/jira/browse/YUNIKORN-397)

##########
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",

Review comment:
       Please clarify that this is from a cached app while post processing or something like that.

##########
File path: .gitignore
##########
@@ -1,3 +1,4 @@
 .idea
 _output/
+vendor

Review comment:
       Nothing generates or should generate a vendor directory in the build. Why do we need this?

##########
File path: pkg/appmgmt/general/general.go
##########
@@ -34,6 +33,8 @@ import (
 	"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"
+
+	"go.uber.org/zap"

Review comment:
       Not sure why we need this change...

##########
File path: pkg/appmgmt/appmgmt.go
##########
@@ -47,7 +47,10 @@ func NewAMService(amProtocol interfaces.ApplicationManagementProtocol,
 		managers:    make([]interfaces.AppManager, 0),
 	}
 
+	log.Logger().Info("Initializing new AppMgmt service")
+
 	if !apiProvider.IsTestingMode() {
+		log.Logger().Info("Registered Spark operator with the AppMgmt service")

Review comment:
       Looking at the register call below we do more than the Spark operator.
   NIT: until we return from the register call the work has not been done yet (registered vs registering).

##########
File path: pkg/appmgmt/sparkoperator/spark.go
##########
@@ -51,6 +40,7 @@ type Manager struct {
 }
 
 func NewManager(amProtocol interfaces.ApplicationManagementProtocol, apiProvider client.APIProvider) *Manager {
+	log.Logger().Info("New AppMgr initialized")

Review comment:
       Isn't the app manager initialised in `ServiceInit()` below?
   You log almost the same message in that call.

##########
File path: pkg/appmgmt/general/general.go
##########
@@ -301,12 +311,14 @@ func (os *Manager) ListApplications() (map[string]interfaces.ApplicationMetadata
 
 	// get existing apps
 	existingApps := make(map[string]interfaces.ApplicationMetadata)
-	if appPods != nil && len(appPods) > 0 {
+	if len(appPods) > 0 {

Review comment:
       range can handle a nil input so there is no need to check at all.

##########
File path: pkg/appmgmt/sparkoperator/spark.go
##########
@@ -105,125 +87,26 @@ func (os *Manager) Stop() {
 	os.stopCh <- struct{}{}
 }
 
-func (os *Manager) getTaskMetadata(pod *v1.Pod) (interfaces.TaskMetadata, bool) {
-	// spark executors are having a common label
-	if appName, ok := pod.Labels[SparkAppNameLabel]; ok {
-		return interfaces.TaskMetadata{
-			ApplicationID: appName,
-			TaskID:        string(pod.UID),
-			Pod:           pod,
-		}, true
-	}
-	return interfaces.TaskMetadata{}, false
-}
-
-func (os *Manager) getAppMetadata(sparkApp *v1beta2.SparkApplication) interfaces.ApplicationMetadata {
-	// extract tags from annotations
-	tags := make(map[string]string)
-	for annotationKey, annotationValue := range sparkApp.GetAnnotations() {
-		tags[annotationKey] = annotationValue
-	}
-
-	// set queue name if app labels it
-	queueName := constants.ApplicationDefaultQueue
-	if an, ok := sparkApp.Labels[constants.LabelQueueName]; ok {
-		queueName = an
-	}
-
-	// retrieve the namespace info from the CRD
-	tags[constants.AppTagNamespace] = sparkApp.Namespace
-
-	return interfaces.ApplicationMetadata{
-		ApplicationID: sparkApp.Name,
-		QueueName:     queueName,
-		User:          "default",
-		Tags:          tags,
-	}
-}
-
-// list all existing applications
-func (os *Manager) ListApplications() (map[string]interfaces.ApplicationMetadata, error) {
-	lister := os.crdInformerFactory.Sparkoperator().V1beta2().SparkApplications().Lister()
-	sparkApps, err := lister.List(labels.Everything())
-	if err != nil {
-		return nil, err
-	}
-
-	existingApps := make(map[string]interfaces.ApplicationMetadata)
-	for _, sparkApp := range sparkApps {
-		existingApps[sparkApp.Name] = os.getAppMetadata(sparkApp)
-	}
-	return existingApps, nil
-}
-
-func (os *Manager) GetExistingAllocation(pod *v1.Pod) *si.Allocation {
-	if meta, valid := os.getTaskMetadata(pod); valid {
-		return &si.Allocation{
-			AllocationKey:    string(pod.UID),
-			AllocationTags:   nil,
-			UUID:             string(pod.UID),
-			ResourcePerAlloc: common.GetPodResource(pod),
-			QueueName:        utils.GetQueueNameFromPod(pod),
-			NodeID:           pod.Spec.NodeName,
-			ApplicationID:    meta.ApplicationID,
-			PartitionName:    constants.DefaultPartition,
-		}
-	}
-	return nil
-}
-
-// callbacks for SparkApplication CRD
-func (os *Manager) addApplication(obj interface{}) {
-	app := obj.(*v1beta2.SparkApplication)
-	log.Logger().Info("spark app added", zap.Any("SparkApplication", app))
-	os.amProtocol.AddApplication(&interfaces.AddApplicationRequest{
-		Metadata: os.getAppMetadata(app),
-	})
-}
-
 func (os *Manager) updateApplication(old, new interface{}) {
 	appOld := old.(*v1beta2.SparkApplication)
 	appNew := new.(*v1beta2.SparkApplication)
+	currState := appNew.Status.AppState.State
 	log.Logger().Debug("spark app updated",
 		zap.Any("old", appOld),
-		zap.Any("old", appNew))
+		zap.Any("new", appNew))
+	log.Logger().Debug("spark app state",
+		zap.String("current state", string(currState)))

Review comment:
       please combine the two log entries into one

##########
File path: pkg/cache/placeholder_manager.go
##########
@@ -46,10 +47,13 @@ type PlaceholderManager struct {
 }
 
 var placeholderMgr *PlaceholderManager
+var placeholderManagerLock sync.Mutex

Review comment:
       The init and usage of the manager has not changed. Having a lock like this seems weird because we should only ever the one that was created by the current test, or when the context is created in production.
   This probably also explains why you needed the nil check in the app code before.
   This is a test problem. By adding the manager usage to the fail application test we are re-using the manager from a previous test. We should not be doing that. Please check the way `TestTryReserve()` and `TestTryReservePostRestart()` initiate the manager.
   It looks like the test context does not create a manager and we might not clean up correctly either

##########
File path: pkg/cache/application.go
##########
@@ -511,11 +517,24 @@ func (app *Application) handleRejectApplicationEvent(event *fsm.Event) {
 		fmt.Sprintf("application %s is rejected by scheduler", app.applicationID)))
 }
 
+func placeholderCleanup(app *Application) {
+	placeholderManager := getPlaceholderManager()
+	if placeholderManager != nil {
+		placeholderManager.cleanUp(app)
+	}
+}
+

Review comment:
       `getPlaceholderManager()` cannot return a nil we can simplify this by directly calling this inside the go function while handling the events:
   ```
   go func() {
       getPlaceholderManager().cleanup(app)
   }
   ```




-- 
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



[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #253: [YUNIKORN-558] Integration with Spark K8s Operator

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #253:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/253#issuecomment-819214180


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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 [#253](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (105b043) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `0.11%`.
   > The diff coverage is `61.29%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #253      +/-   ##
   ==========================================
   + Coverage   59.75%   59.86%   +0.11%     
   ==========================================
     Files          35       36       +1     
     Lines        3133     3294     +161     
   ==========================================
   + Hits         1872     1972     +100     
   - Misses       1180     1241      +61     
     Partials       81       81              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?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/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
   | [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
   | [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `65.43% <0.00%> (-16.11%)` | :arrow_down: |
   | [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
   | [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
   | [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `55.55% <31.25%> (+2.49%)` | :arrow_up: |
   | [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `33.33% <40.00%> (+3.33%)` | :arrow_up: |
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/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-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `39.72% <45.45%> (-1.36%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [adf66f9...105b043](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/253?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org