[GitHub] [flink] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-717725432 Merging ... 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-717723501 The Flink CI has passed as well: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=8382=results 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-717332311 The CI for your latest push has failed because of some transient network error in our build environment. This stuff sometimes happens. I manually triggered a re-start of the failed builds. I've pushed your latest commit to my personal azure account, and it has passed: https://dev.azure.com/rmetzger/Flink/_build/results?buildId=8539=results 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-717209168 Yes, this comment is tracking the build results: https://github.com/apache/flink/pull/13457#issuecomment-697012704 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-717148013 Thanks. The CI system should pick up your changes and verify them. Let's wait and see. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-716746473 I guess you can set the env value here: https://github.com/apache/flink/blob/master/flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java#L689 Let me know if you don't have time to address this (I would also understand if you are a bit annoyed by this lengthy review) .. I can also do the last step if you want. I would like this change to be included in the Flink 1.12 release. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-714562274 Thanks for the push. It seems that the YARN test is failing because of environment variable is not available in the tests. It's probably easy to add to the YarnTestBase. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-713381865 For some reason, the environment variable was set when I tested it, not sure what was going on (I'm on macos with bash 5.0.17), maybe different behavior? I agree to all your proposals! 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-713102468 So I'd say, let's re-introduce the config param, add it to the log4j.properties and then we'll finally merge this change. I quite like this now, as it does the rotation stuff in log4j, not using some home-cooked bash stuff. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-713101853 (Sry, hit the wrong button -- it's too late 洛 ) 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457:
URL: https://github.com/apache/flink/pull/13457#issuecomment-713101595
I'm really sorry that I'm only responding to you every 5 days. I'd actually
like to keep a certain momentum going with this change. I'll try to get back
faster!
I had an offline discussion with @zentol about the configuration parameter.
Since it is documented, we treat it as public API, which we cannot break
between minor releases.
However, he also wondered why we can't forward the configured FLINK_LOG_MAX
value to the log4j settings?
```
appender.main.policies.type = Policies
appender.main.policies.size.type = SizeBasedTriggeringPolicy
appender.main.policies.size.size = 100MB
appender.main.policies.startup.type = OnStartupTriggeringPolicy
appender.main.strategy.type = DefaultRolloverStrategy
appender.main.strategy.max = ${env:MAX_LOG_FILE_NUMBER:-10}
```
I have tried this locally and it seems to work.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-708604644 If we are going to remove the config param: There are a few more usages of it in `config.sh`. But let's wait for Zentol first. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-708603602 > The log4j-cli.properties logs to a file and doesn't use a rolling policy, I can change that to rolling. Let's leave that as-is. The logfile is overwritten on every cli start anyways. > The log4j-console.properties uses the rolling policy and max files to keep is set to 10. Maybe it makes sense to do the same for log4j.properties. Yes, I agree. That way, we bound the log files directory size to roughly 1GB, which is a good safety net. > Then I noticed that there is a CoreOptions.FLINK_LOG_MAX config parameter, is it save to remove it? This will not be used anymore. @zentol are we considering these config options part of the stable API? I don't think this setting is used much, if at all. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-703604630 That sounds like a good plan. The more complexity we can remove from the bash scripts, the better. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-700010022 I believe you also need to adopt the `config.sh#rotateLogFile` function, as it rotates existing logs. (basically when you run start-cluster, stop-cluster, start-cluster, the old log files will have a `.1` in the end) With your change, now the .1 file is empty on the second start. I don't have a good idea how to solve this right away. 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] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-76536 The change works with the web UI ✅ The rolling works (tested it with 1MB instead of 100MB ) ✅ In my opinion, we should add the rolling also to `log4j-session.properties`, so that YARN and K8s sessions use it 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] rmetzger commented on pull request #13457: [FLINK-8357] Use rolling logs as default
rmetzger commented on pull request #13457: URL: https://github.com/apache/flink/pull/13457#issuecomment-69985 Thanks a lot for your pull request. I'll soon take a look! 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
