You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Presently, atMostEvery only supports int and java.util.concurrent.TimeUnit. However, the parameters become inconvenient to use for java.time.Duration. Additionally, when using java.time.Duration#toNanos and TimeUnit#NANOSECONDS, the field gets truncated to an int`. This causes the implementer of the duration field to explicitly field depth to avoid overflow, which can become awkward.
I would like atMostEvery to support java.time.Duration, rather than it's present args. This would make more sense on an usability standpoint of this API.
The text was updated successfully, but these errors were encountered:
Sadly Duration is a recent API and adding support for it would break users on older JDKs.
The reason that it was originally "int" for the value is that it was expected that users would hardcode a value into the log statement, and we wouldn't expect manually chosen values to go out of range (after all "atMostEvery(18000000000000, NANOSECONDS)" is not readable compared to "atMostEvery(5, HOURS)").
What's your use case by which a Duration is needed? Is it a non-constant value for some reason (this is permitted but unusual).
(of course by "recent" I mean Java 8, but Flogger predates that and was for a long time 1.6 compatible).
The other reason something like Duration wasn't used is that using a split count/unit approach means that there's no allocation at the call site, which helps when logging is rate limited.
A log statement containing:
atMostEvery(Duration.ofHours(2))
in an inner loop could make potentially millions of Duration instances to pass into the log statement.
And once you decide to cache that value (e.g. in a static field) you get:
atMostEvery(TWO_HOURS)
which is hardly different to:
atMostEvery(2, HOURS)
I'm not saying it'll never happen, but it will mean bumping Flogger to Java 8, and we'll need to understand who that might break.
Presently,
atMostEvery
only supportsint
andjava.util.concurrent.TimeUnit
. However, the parameters become inconvenient to use forjava.time.Duration
. Additionally, when usingjava.time.Duration#toNanos
andTimeUnit#NANOSECONDS, the field gets truncated to an
int`. This causes the implementer of the duration field to explicitly field depth to avoid overflow, which can become awkward.I would like
atMostEvery
to supportjava.time.Duration
, rather than it's present args. This would make more sense on an usability standpoint of this API.The text was updated successfully, but these errors were encountered: