[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-05-17 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-629911292


   @zentol Thanks a lot for the `fixup`.
   I have rebased the latest master and force pushed.



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

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




[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-05-14 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-629044350


   @zentol I have updated the PR according to the discussion. We will start 
with `.log` file by using rolling appender. I think it could meet the most 
requirements and leave the open question about `.out` and `.err` file later.



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

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




[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-05-11 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-626617247


   You are right. From our internal use case, the biggest problem is `.log` 
could not be accessed via web dashboard. So maybe we could start with this and 
leave the `.out` file later until we reach a consensus or find a reasonable 
solution.
   
   Moreover, i suggest to use the `RollingFileAppender` as default, set 
`MaxFileSize` to 100MB and `MaxBackupIndex` to 10, which means the logging file 
will consume 1G disk at most.
   
   @zentol @tillrohrmann What do you think?



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

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




[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-05-07 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-625138407


   Since we use tee to redirect the stdout to `out` file, it is hard to support 
rolling file and control the file size. This is also a reason for why we could 
consider the stream redirection. No duplication, file size control and better 
readability. 



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

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




[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-05-06 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-625042466


   I also prefer to use the `ConsoleAppender` in log4j. It is more 
straightforward and stable.
   
   From the discussion in the ML, it seems that the only concern is about the 
disk consumption. So do we need to use the `RollingFileAppender` by default in 
`log4j-console.properties` and set the `MaxFileSize` and `MaxBackupIndex` 
explicitly?
   
   @tillrohrmann @zentol WDYT?



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

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




[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-04-27 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-620386953


   @zentol If we define the `.out` output as what would be printed to the 
console(including the logging redirection), then this is indeed the expected 
behavior. After a careful consideration, i tend to agree with you that **NOT** 
introduce the stream redirection.
   
   So i will remove the `StdOutErrRedirector` and related log4j/logback 
configuration. After that things become simpler. We just need to update the 
script and log4j/logback configuration. Given this, i will force push the new 
changes. Thanks a lot for your review and suggestion.



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

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




[GitHub] [flink] wangyang0918 commented on pull request #11839: [FLINK-17166][dist] Modify the log4j-console.properties to also output logs into the files for WebUI

2020-04-25 Thread GitBox


wangyang0918 commented on pull request #11839:
URL: https://github.com/apache/flink/pull/11839#issuecomment-619471329


   @zentol Thanks a lot for your review.
   
   I agree with you that introducing the stream redirections may take some 
problems. So we need to be very careful to enable the redirection via 
`StdOutErrRedirector.redirectStdOutErr`. Currently, i only see the requirements 
when we want to output the stdout/err to log files and console at the same 
time. Also i do not have seen a better solution. Using the `tee` will make us 
have to use the `grep` to filter out the log4j loggings. Just as the discussion 
in the 
[ticket](https://issues.apache.org/jira/browse/FLINK-17166?focusedCommentId=17087643=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17087643).
   
   Also not all entrypoints need to do this. I think only the docker 
environment deployment need(e.g. standalone, K8s). We could put it in the first 
line in the main of `XXEntrypoint` and i do not find a logging system cache 
problem here.
   
   The most important concerns when i have to introduce the stream redirection 
is not about the mixing the `.out` and `.err` file. It is we have to filter out 
the log4j logging from the stdout/err when using `tee`.
   Actually, the YARN and K8s deployment always put the stdout to `.out` file 
and stderr to `.err` file. And i think it is better to separate them.
   



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

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