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