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 2022/01/21 11:48:17 UTC

[GitHub] [incubator-yunikorn-core] anuraagnalluri opened a new pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

anuraagnalluri opened a new pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365


   ### What is this PR for?
   Location of the state dump file should be configurable instead of hardcoded. PR also provides a default retention policy to ensure the file does not grow unbounded.   
   
   ### What type of PR is it?
   * [ ] - Bug Fix
   * [x] - Improvement
   * [ ] - Feature
   * [ ] - Documentation
   * [ ] - Hot Fix
   * [ ] - Refactoring
   
   ### Todos
   * [ ] - Need to add more tests in handlers_test.go
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/YUNIKORN-949
   
   ### How should this be tested?
   * Modified tests in handlers_test.go and manual testing
   
   ### Screenshots (if appropriate)
   
   ### Questions:
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   hi @anuraagnalluri thanks. @pbacsko are we good to go for this one?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] pbacsko commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791597303



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +160,48 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath()
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,
+	})
+
+	if err = stateLog.Output(2, string(prettyJSON)); err != nil {

Review comment:
       2 looks like a magic number, pls extract to const

##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +160,48 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath()
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,

Review comment:
       shouldn't these two be configurable as well?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r793365629



##########
File path: pkg/webservice/handlers_test.go
##########
@@ -1342,19 +1343,54 @@ func TestGetFullStateDump(t *testing.T) {
 	resp := &MockResponseWriter{}
 
 	getFullStateDump(resp, req)
-	receivedBytes := resp.outputBytes
 	statusCode := resp.statusCode
-	assert.Assert(t, len(receivedBytes) > 0, "json response is empty")
+	fi, err := os.Stat(defaultStateDumpFilePath)
+	assert.NilError(t, err)
+	assert.Assert(t, fi.Size() > 0, "json response is empty")
 	assert.Check(t, statusCode != http.StatusInternalServerError, "response status code")
 	var aggregated AggregatedStateInfo
+	receivedBytes, err := ioutil.ReadFile(defaultStateDumpFilePath)
+	assert.NilError(t, err)
+	err = json.Unmarshal(receivedBytes, &aggregated)
+	assert.NilError(t, err)
+	verifyStateDumpJSON(t, &aggregated)
+}
+
+func TestGetFullStateDumpNonDefaultPath(t *testing.T) {
+	stateDumpFilePath := "tmp/non-default-yunikorn-state.txt"
+	defer deleteStateDumpDir(t)
+	schedulerContext = prepareSchedulerContext(t)
+	defer deleteStateDumpFile(t, schedulerContext)
+
+	partitionContext := schedulerContext.GetPartitionMapClone()
+	context := partitionContext[normalizedPartitionName]
+	app := newApplication("appID", normalizedPartitionName, "root.default", rmID)
+	err := context.AddApplication(app)
+	assert.NilError(t, err, "failed to add Application to partition")
+
+	imHistory = history.NewInternalMetricsHistory(5)
+	req, err2 := http.NewRequest("GET", "/ws/v1/getfullstatedump", strings.NewReader(""))
+	assert.NilError(t, err2)
+	req = mux.SetURLVars(req, make(map[string]string))
+	resp := &MockResponseWriter{}
+
+	getFullStateDump(resp, req)
+	statusCode := resp.statusCode
+	fi, err := os.Stat(stateDumpFilePath)

Review comment:
       This line will fail since `tmp/yunikorn-state.txt` doesn't exist. The `stateDumpFilePath` needs to be reflected in the `context` this test is reading. Trying to figure out the best way to set `context.StateDumpFilePath` for this test. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791380193



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,7 +160,20 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)

Review comment:
       Good point -- this may be wasteful. However, the log object requires being constructed from the response writer which is why I had it in this method. I'll familiarize myself more with how the response writer gets passed from `routes.go` and see if I can change this.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] pbacsko merged pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
pbacsko merged pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020075314


   @yangwwei The added unit test works, and I've verified the environment variable to set the dump file path is properly sourced from the configmap. Relevant PRs [for k8sshim](https://github.com/apache/incubator-yunikorn-k8shim/pull/358) and [release](https://github.com/apache/incubator-yunikorn-release/pull/70).
   
   However, when I edit the configmap, I don't see the environment variable change in the scheduler pod even after 30 min. I read about config hot refresh [here](https://yunikorn.apache.org/docs/0.8.0/setup/configure_scheduler/#configuration-hot-refresh), but am missing a few details. Is the hot refresh enabled by default, and if so, how long does it typically take? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head 0aa8fcc. Consider uploading reports for the commit 0aa8fcc to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...0aa8fcc](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   > Just have a question - this file path is independent of the partitions, so it feels more logical to place this in `SchedulerConfig` instead of `PartitionConfig`. I know @yangwwei recommended this as a partition-level setting, just wondering if lifting it one level further has more drawbacks.
   
   Right now, we do not have a global "schedulerConfig" field, all configs are partition-based. It is not a big problem now because we only support a single partition by far.  I agree it is more logical to have some global config to config this, but it is not something that should be done in this PR. So current approach looks good.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head 72e63bf. Consider uploading reports for the commit 72e63bf to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...72e63bf](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791276416



##########
File path: go.mod
##########
@@ -46,6 +46,7 @@ require (
 	golang.org/x/text v0.3.2 // indirect
 	golang.org/x/tools v0.0.0-20200415000939-92398ad77b89 // indirect
 	google.golang.org/grpc v1.26.0
+	gopkg.in/natefinch/lumberjack.v2 v2.0.0

Review comment:
       MIT license, good to go
   Just did a little more research, golang doesn't have a very standard way to do rolling logs, this library seems to be a reasonable choice. Also noticed this lib was mentioned in Zap's doc: https://github.com/uber-go/zap/blob/master/FAQ.md#does-zap-support-log-rotation.

##########
File path: pkg/webservice/state_dump.go
##########
@@ -170,16 +186,21 @@ func startBackGroundStateDump(period time.Duration) error {
 
 	if periodicStateDump {
 		var errMsg = "state dump already running"
-		log.Logger().Error(errMsg)
+		yunikornLog.Logger().Error(errMsg)
 		return fmt.Errorf(errMsg)
 	}
 
+	stateDumpFilePath, exists := os.LookupEnv(stateDumpFilePathEnvKey)
+	if !exists {
+		stateDumpFilePath = defaultStateDumpFilePath
+	}

Review comment:
       We should avoid dup code, this is the same as 166 - 169
   if this is for dynamic reloading new ENV, can we have a convenient function `getStateDumpFilePath()`

##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,7 +160,20 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)

Review comment:
       do we need to new a state log every time?
   should we just reuse the same object and use it to write the state data over time?
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b38fe25) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `76.92%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9021      +15     
   ==========================================
   + Hits         6085     6100      +15     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.73% <76.92%> (+2.20%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...b38fe25](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   hi @anuraagnalluri thanks for the PR, I did a quick review. @pbacsko please help to review this patch.
   
   >However, when I edit the configmap, I don't see the environment variable change in the scheduler pod even after 30 min. I read about config hot refresh here, but am missing a few details. Is the hot refresh enabled by default, and if so, how long does it typically take?
   
   Yes, it is enabled by default. YK loads configs from the configmap, and it keeps watching the configmap object, if some fields changes, YK does a reload and load all new values to memory.  But here it seems to be a bit different. Because you are directly fetching ENV var using the os library, you can only expect that work unless the ENV updates got propagated to the docker container.  And that is very unlikely not the case, you can see a similar post [here](https://stackoverflow.com/questions/45050050/can-i-modify-containers-environment-variables-without-restarting-pod-using-kube). If we want this to be a config supports hot-refresh, it cannot be a ENV var, it can be a partition level config property in the configmap, and it needs a bit more effort to load/reload that property on changes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r794202628



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,7 +160,20 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)

Review comment:
       @yangwwei Maybe I'm missing something obvious but I couldn't get this working after several hours. I tried to use `zapcore` directly in `webservices.go` so we only instantiate one logger in `NewWebApp` as per the [zap/lumberjack documentation](https://github.com/uber-go/zap/blob/master/FAQ.md#does-zap-support-log-rotation).
   
   However, it seems zap must log the message string as a value with `MessageKey` as per its [EncoderConfig](https://pkg.go.dev/go.uber.org/zap/zapcore#EncoderConfig). Then, I tried to unmarshal the nested JSON to a nested struct (ex. outer struct containing [this struct](https://github.com/apache/incubator-yunikorn-core/blob/master/pkg/webservice/state_dump.go#L51)). Problem with this is that the logged message is still surrounded by `"`, so it doesn't unmarshal in to the appropriate type (only string).
   
   Happy to take another stab at it if you have any pointers, or want me to send a commit with screenshots of the zap log output to get a better idea of the issue. Functionally, things are working fine as per [YUNIKORN-949](https://issues.apache.org/jira/browse/YUNIKORN-949).




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1023882282


   @yangwwei As discussed, I made the state dump file path a partition-level config parameter. This works as expected during testing (hot refresh takes effect and location of the output file changes after 2 minutes). 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   hi @anuraagnalluri thanks for the PR, I did a quick review. @pbacsko please help to review this patch.
   
   >However, when I edit the configmap, I don't see the environment variable change in the scheduler pod even after 30 min. I read about config hot refresh here, but am missing a few details. Is the hot refresh enabled by default, and if so, how long does it typically take?
   
   Yes, it is enabled by default. YK loads configs from the configmap, and it keeps watching the configmap object, if some fields changes, YK does a reload and load all new values to memory.  But here it seems to be a bit different. Because you are directly fetching ENV var using the os library, you can only expect that work unless the ENV updates got propagated to the docker container.  And that is very unlikely not the case, you can see a similar post [here](https://stackoverflow.com/questions/45050050/can-i-modify-containers-environment-variables-without-restarting-pod-using-kube). If we want this to be a config supports hot-refresh, it cannot be a ENV var, it can be a partition level config property in the configmap, and it needs a bit more effort to load/reload that property on changes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020839615


   @yangwwei Understood, thank you for the explanation. I will try to change this to a partition level config property. Since the code for YK's monitoring of configmap object already exists, I assume I'd just need to include additional logic to set/unset the env var for `STATE_DUMP_FILE_PATH` based on existence of the config property. I'll modify the PR to reflect that soon. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] pbacsko commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791597303



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +160,48 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath()
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,
+	})
+
+	if err = stateLog.Output(2, string(prettyJSON)); err != nil {

Review comment:
       2 looks like a magic number, pls extract to const




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020075314


   @yangwwei The added unit test works, and I've verified the environment variable to set the dump file path is properly sourced from the configmap. Relevant PRs [for k8sshim](https://github.com/apache/incubator-yunikorn-k8shim/pull/358) and [for release](https://github.com/apache/incubator-yunikorn-release/pull/70).
   
   However, when I edit the configmap, I don't see the environment variable change in the scheduler pod even after 30 min. I read about config hot refresh [here](https://yunikorn.apache.org/docs/0.8.0/setup/configure_scheduler/#configuration-hot-refresh), but am missing a few details. Is the hot refresh enabled by default, and if so, how long does it typically take? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b38fe25) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `76.92%`.
   
   > :exclamation: Current head b38fe25 differs from pull request most recent head 55709ea. Consider uploading reports for the commit 55709ea to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9021      +15     
   ==========================================
   + Hits         6085     6100      +15     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.73% <76.92%> (+2.20%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...55709ea](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791380193



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,7 +160,20 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)

Review comment:
       Good point -- this may be wasteful. However, the log object requires being constructed from the response writer which is why I had it in this method. I'll familiarize myself more with how the response writer gets passed from `routes.go` and see if I can change this.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r794064670



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +161,52 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath(schedulerContext)
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,
+	})
+
+	if err = stateLog.Output(stateLogCallDepth, string(prettyJSON)); err != nil {
 		return err
 	}
 
 	return nil
 }
 
+func getStateDumpFilePath(cc *scheduler.ClusterContext) string {
+	cc.RLock()
+	defer cc.RUnlock()
+
+	for _, partition := range cc.GetPartitionMapClone() {
+		if partition.GetStateDumpFilePath() != "" {
+			return partition.GetStateDumpFilePath()
+		}

Review comment:
       Since state dump returns info of all partitions, but the state dump output path is a partition-level config property, we iterate through partitions until we find first occurrence of the property. This first occurrence will be used as the output path if multiple values are configured for different partitions. If no occurrences, we default to `yunikorn-state.txt`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020839615


   @yangwwei Understood, thank you for the explanation. I will try to change this to a partition level config property. Since the code for YK's monitoring of configmap object already exists, I assume I'd just need to include additional logic to set/unset the env var for `STATE_DUMP_FILE_PATH` based on existence of the config property. I'll modify the PR to reflect that soon. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] pbacsko commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
pbacsko commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1030597855


   Ah sorry guys, for some reason, I thought that Weiwei had already merged this.
   
   +1 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head b38fe25. Consider uploading reports for the commit b38fe25 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...b38fe25](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head c8217e7. Consider uploading reports for the commit c8217e7 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...c8217e7](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] pbacsko commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r794749623



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +160,48 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath()
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,

Review comment:
       Fine, OK for me.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head 72e63bf. Consider uploading reports for the commit 72e63bf to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...72e63bf](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r798133792



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +161,52 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath(schedulerContext)
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,
+	})
+
+	if err = stateLog.Output(stateLogCallDepth, string(prettyJSON)); err != nil {
 		return err
 	}
 
 	return nil
 }
 
+func getStateDumpFilePath(cc *scheduler.ClusterContext) string {
+	cc.RLock()
+	defer cc.RUnlock()
+
+	for _, partition := range cc.GetPartitionMapClone() {
+		if partition.GetStateDumpFilePath() != "" {
+			return partition.GetStateDumpFilePath()
+		}

Review comment:
       > Just have a question - this file path is independent of the partitions, so it feels more logical to place this in `SchedulerConfig` instead of `PartitionConfig`. I know @yangwwei recommended this as a partition-level setting, just wondering if lifting it one level further has more drawbacks.
   
   Right now, we do not have a global "schedulerConfig" field, all configs are partition-based. It is not a big problem now because we only support a single partition by far.  I agree it is more logical to have some global config to config this, but it is not something that should be done in this PR. So current approach looks good.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1028503539


   @yangwwei Thank you, yes I created a JIRA for documenting new config and assigned it to myself: https://issues.apache.org/jira/browse/YUNIKORN-1068.
   
   Also @pbacsko, here's the follow-up for making log rotation params configurable: https://issues.apache.org/jira/browse/YUNIKORN-1069


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1019590198


   @yangwwei @pbacsko First screenshot is example of how state dump files look after scheduler was running idly for 3+ days. In this PR, we cap the number of max files at 10 consisting of 10MB each using a log rotator pkg called `Lumberjack`.
   
   Second image is monitoring the file size of the current file we're writing to. We always write to the non-timestamped default `yunikorn-state.txt` or user supplied path + filename via configmap as Peter recommended. When we reach file size limit (10MB in this case), `Lumberjack` will timestamp the file, delete any old ones if the max file number limit is reached, and start logging to a new non-timestamped file.
   
   Third image is just a limited view of the JSON output in one of these files. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...4a35d94](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791276416



##########
File path: go.mod
##########
@@ -46,6 +46,7 @@ require (
 	golang.org/x/text v0.3.2 // indirect
 	golang.org/x/tools v0.0.0-20200415000939-92398ad77b89 // indirect
 	google.golang.org/grpc v1.26.0
+	gopkg.in/natefinch/lumberjack.v2 v2.0.0

Review comment:
       MIT license, good to go
   Just did a little more research, golang doesn't have a very standard way to do rolling logs, this library seems to be a reasonable choice. Also noticed this lib was mentioned in Zap's doc: https://github.com/uber-go/zap/blob/master/FAQ.md#does-zap-support-log-rotation.

##########
File path: pkg/webservice/state_dump.go
##########
@@ -170,16 +186,21 @@ func startBackGroundStateDump(period time.Duration) error {
 
 	if periodicStateDump {
 		var errMsg = "state dump already running"
-		log.Logger().Error(errMsg)
+		yunikornLog.Logger().Error(errMsg)
 		return fmt.Errorf(errMsg)
 	}
 
+	stateDumpFilePath, exists := os.LookupEnv(stateDumpFilePathEnvKey)
+	if !exists {
+		stateDumpFilePath = defaultStateDumpFilePath
+	}

Review comment:
       We should avoid dup code, this is the same as 166 - 169
   if this is for dynamic reloading new ENV, can we have a convenient function `getStateDumpFilePath()`

##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,7 +160,20 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)

Review comment:
       do we need to new a state log every time?
   should we just reuse the same object and use it to write the state data over time?
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] pbacsko commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r791597611



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +160,48 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath()
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,

Review comment:
       shouldn't these two be configurable as well?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...4a35d94](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1028490460


   @pbacsko Thanks for the recommendation. @yangwwei can you advise? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head 4ab2768. Consider uploading reports for the commit 4ab2768 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...4ab2768](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020717823


   hi @anuraagnalluri thanks for the PR, I did a quick review. @pbacsko please help to review this patch.
   
   >However, when I edit the configmap, I don't see the environment variable change in the scheduler pod even after 30 min. I read about config hot refresh here, but am missing a few details. Is the hot refresh enabled by default, and if so, how long does it typically take?
   
   Yes, it is enabled by default. YK loads configs from the configmap, and it keeps watching the configmap object, if some fields changes, YK does a reload and load all new values to memory.  But here it seems to be a bit different than what you expected. Because you are directly fetching ENV var using the os library, you can only expect that work unless the ENV updates got propagated to the docker container.  And that is very unlikely not the case, you can see a similar post [here](https://stackoverflow.com/questions/45050050/can-i-modify-containers-environment-variables-without-restarting-pod-using-kube). If we want this to be a config supports hot-refresh, it cannot be a ENV var, it can be a partition level config property in the configmap, and it needs a bit more effort to load/reload that property on changes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020717823






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] yangwwei edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
yangwwei edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1020717823






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (55709ea) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.10%`.
   > The diff coverage is `79.31%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.66%   +0.10%     
   ==========================================
     Files          64       64              
     Lines        9006     9053      +47     
   ==========================================
   + Hits         6085     6126      +41     
   - Misses       2685     2689       +4     
   - Partials      236      238       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/common/configs/config.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZy5nbw==) | `76.66% <ø> (ø)` | |
   | [pkg/common/configs/configvalidator.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL2NvbW1vbi9jb25maWdzL2NvbmZpZ3ZhbGlkYXRvci5nbw==) | `86.97% <68.42%> (-1.34%)` | :arrow_down: |
   | [pkg/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `80.12% <79.31%> (+2.59%)` | :arrow_up: |
   | [pkg/scheduler/partition.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3NjaGVkdWxlci9wYXJ0aXRpb24uZ28=) | `74.26% <100.00%> (+0.25%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...55709ea](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#issuecomment-1028503539


   @yangwwei Thank you, yes I will create a JIRA for documenting new config and assign it to myself.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] anuraagnalluri commented on a change in pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-core/pull/365#discussion_r794051574



##########
File path: pkg/webservice/state_dump.go
##########
@@ -157,29 +160,48 @@ func doStateDump(w io.Writer) error {
 	if err != nil {
 		return err
 	}
-	if _, err = w.Write(prettyJSON); err != nil {
+
+	stateLog := log.New(w, "", 0)
+	stateDumpFilePath := getStateDumpFilePath()
+	stateLog.SetOutput(&lumberjack.Logger{
+		Filename:   stateDumpFilePath,
+		MaxSize:    10,
+		MaxBackups: 10,

Review comment:
       I'd like to leave as hardcoded for now if that's ok and file a follow-up JIRA to make them configurable.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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

[GitHub] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #365: [YUNIKORN-949] Location of the state dump file should be configurable

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


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a35d94) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/137803a63bc84cd5dc6b08722bfafd116d40b25a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (137803a) will **increase** coverage by `0.05%`.
   > The diff coverage is `77.77%`.
   
   > :exclamation: Current head 4a35d94 differs from pull request most recent head 4b3ccf1. Consider uploading reports for the commit 4b3ccf1 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&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-core/pull/365?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     #365      +/-   ##
   ==========================================
   + Coverage   67.56%   67.62%   +0.05%     
   ==========================================
     Files          64       64              
     Lines        9006     9022      +16     
   ==========================================
   + Hits         6085     6101      +16     
     Misses       2685     2685              
     Partials      236      236              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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/webservice/state\_dump.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365/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-cGtnL3dlYnNlcnZpY2Uvc3RhdGVfZHVtcC5nbw==) | `79.87% <77.77%> (+2.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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-core/pull/365?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 [137803a...4b3ccf1](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/365?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.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

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