-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatic purge of daemon logs #213
Conversation
* Origin file: | ||
* https://github.com/apache/camel/blob/4ea9e6c357371682b855d2d79655b41120331b7a/core/camel-util/src/main/java/org/apache/camel/util/TimeUtils.java | ||
*/ | ||
public final class TimeUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TimeUtils brings quite a lot of code. I wonder whether I cannot be simplified or perhaps fully replaced by java.time.Duration.parse(CharSequence)
/java.time.Duration.toString()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for a tiny mvnd
executable, but I don't think our code size will become a problem at the moment. When the executable has been trimmed (I'll raise an issue with some infos), we could revisit if needed.
The goal is to be human readable. The java.time.Duration.parse
/toString
methods use the ISO format which looks like PT8H6M12.345S
, and this is not friendly at all imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code could be simplified by using a single regex instead of multiple ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've double checked and the mvnd
executable already contains the java.time
packages, mainly because the java.lang.ProcessHandleImpl$Info.toString()
references it indirectly (in #totalCpuDuration()
). I'll try removing this reference to see how much we gain in the final size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code could be simplified by using a single regex instead of multiple ones.
That would be nice. I even would not mind supporting only the shortest variants 1d
vs. 1 day
, 1 days
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've double checked and the
mvnd
executable already contains thejava.time
packages, mainly because thejava.lang.ProcessHandleImpl$Info.toString()
references it indirectly (in#totalCpuDuration()
). I'll try removing this reference to see how much we gain in the final size.
Interesting excercise :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really easy to get rid of the java.time.*
packages as those are referenced by various classes in the JDK such as java.util.Formatter
, java.util.logging.LogRecord
...
@@ -68,14 +68,18 @@ public String asCommandLineProperty(String value) { | |||
* The number of log lines to display for each Maven module that is built in parallel. | |||
*/ | |||
MVND_ROLLING_WINDOW_SIZE("mvnd.rollingWindowSize", null, "0", false), | |||
/** | |||
* The automatic log purge period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A short description of the supported syntax and a couple of duration examples would be nice here and on other Duration options.
We should slowly start adding @since
so that users see how old is which option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll add one.
I think we can start using @since
when we'll switch to a non 0.0.x
version, which kinda assumes, very early code and non stabilised api.
…ironment class, add a unit test
+ (h != null ? TimeUnit.HOURS.toMillis(Long.parseLong(h)) : 0) | ||
+ (m != null ? TimeUnit.MINUTES.toMillis(Long.parseLong(m)) : 0) | ||
+ (s != null ? TimeUnit.SECONDS.toMillis(Long.parseLong(s)) : 0) | ||
+ (l != null ? TimeUnit.MILLISECONDS.toMillis(Long.parseLong(l)) : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner, thanks!
No description provided.