JAkutenshi commented on code in PR #7287:
URL: https://github.com/apache/ignite-3/pull/7287#discussion_r2645790610
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/Utils.java:
##########
@@ -341,6 +341,34 @@ public static long monotonicUs() {
return TimeUnit.NANOSECONDS.toMicros(System.nanoTime());
}
+ /**
+ * Computes a monotonic-time deadline based on the provided timeout.
+ *
+ * <p>The returned value is expressed in the same time domain as
+ * {@link Utils#monotonicMs()} and is intended for comparisons against
+ * that value (for example, {@code now >= deadline}).
+ *
+ * <p>Timeout semantics:
+ * <ul>
+ * <li>{@code timeoutMillis < 0} or {@code timeoutMillis ==
Long.MAX_VALUE}
+ * — the deadline is unbounded and {@code Long.MAX_VALUE} is
returned.</li>
+ * <li>{@code timeoutMillis >= 0} — the deadline is computed as
+ * {@code Utils.monotonicMs() + timeoutMillis}.</li>
+ * </ul>
+ *
+ * @param timeoutMillis timeout in milliseconds controlling the retry
window
+ * @return a monotonic-time deadline in milliseconds, or {@code
Long.MAX_VALUE}
+ * if retries are unbounded
+ */
+ public static long monotonicMsAfter(long timeoutMillis) {
+ if (timeoutMillis == Long.MAX_VALUE || timeoutMillis < 0) {
+ // Infinite retry.
+ return Long.MAX_VALUE;
+ } else {
+ return monotonicMs() + timeoutMillis;
Review Comment:
Should we do overflow check?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/Utils.java:
##########
@@ -341,6 +341,34 @@ public static long monotonicUs() {
return TimeUnit.NANOSECONDS.toMicros(System.nanoTime());
}
+ /**
+ * Computes a monotonic-time deadline based on the provided timeout.
+ *
+ * <p>The returned value is expressed in the same time domain as
+ * {@link Utils#monotonicMs()} and is intended for comparisons against
+ * that value (for example, {@code now >= deadline}).
+ *
+ * <p>Timeout semantics:
+ * <ul>
+ * <li>{@code timeoutMillis < 0} or {@code timeoutMillis ==
Long.MAX_VALUE}
+ * — the deadline is unbounded and {@code Long.MAX_VALUE} is
returned.</li>
+ * <li>{@code timeoutMillis >= 0} — the deadline is computed as
+ * {@code Utils.monotonicMs() + timeoutMillis}.</li>
+ * </ul>
+ *
+ * @param timeoutMillis timeout in milliseconds controlling the retry
window
+ * @return a monotonic-time deadline in milliseconds, or {@code
Long.MAX_VALUE}
+ * if retries are unbounded
+ */
+ public static long monotonicMsAfter(long timeoutMillis) {
+ if (timeoutMillis == Long.MAX_VALUE || timeoutMillis < 0) {
Review Comment:
Shouldn't we throw `IllegalArgumentException` on negative argument?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/Utils.java:
##########
@@ -341,6 +341,34 @@ public static long monotonicUs() {
return TimeUnit.NANOSECONDS.toMicros(System.nanoTime());
}
+ /**
+ * Computes a monotonic-time deadline based on the provided timeout.
+ *
+ * <p>The returned value is expressed in the same time domain as
+ * {@link Utils#monotonicMs()} and is intended for comparisons against
+ * that value (for example, {@code now >= deadline}).
+ *
+ * <p>Timeout semantics:
+ * <ul>
+ * <li>{@code timeoutMillis < 0} or {@code timeoutMillis ==
Long.MAX_VALUE}
+ * — the deadline is unbounded and {@code Long.MAX_VALUE} is
returned.</li>
+ * <li>{@code timeoutMillis >= 0} — the deadline is computed as
+ * {@code Utils.monotonicMs() + timeoutMillis}.</li>
+ * </ul>
+ *
+ * @param timeoutMillis timeout in milliseconds controlling the retry
window
+ * @return a monotonic-time deadline in milliseconds, or {@code
Long.MAX_VALUE}
+ * if retries are unbounded
+ */
+ public static long monotonicMsAfter(long timeoutMillis) {
Review Comment:
Moreover, we have case mentioned in `RaftGroupServiceImpl#sendWithRetry`:
```
@param sendWithRetryTimeoutMillis Timeout for entire request sending (with
retries) in milliseconds, a negative value means no timeout.
```
So on the one hand we have javadoc and usage contract in
`RaftGroupServiceImpl#snapshot` that implies no timeout, but this condition
breaks it.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -102,7 +102,7 @@ class RetryContext {
* @param originDescription Supplier describing the origin request from
which this one depends, or returning {@code null} if
* this request is independent.
* @param requestFactory Factory for creating requests to the target peer.
- * @param stopTime Timestamp that denotes the point in time up to which
retry attempts will be made.
+ * @param sendWithRetryTimeoutMillis Timeout, in milliseconds, for the
entire retry sequence.
Review Comment:
Also [due to
comment](https://github.com/apache/ignite-3/pull/7287/changes#r2645893606) I
propose to duplicate the javadoc from the caller `#sendWithRetry`.
And extra proposal: to make verbal constants that are signalling that given
value means no timeout or something else. `-1` is a magic value that requires
to figure out what there is happening (I know that it's not your code and that
it isn't the PR intention, that's why this paragraph is just a piggyback
improvement proposal).
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/Utils.java:
##########
@@ -341,6 +341,34 @@ public static long monotonicUs() {
return TimeUnit.NANOSECONDS.toMicros(System.nanoTime());
}
+ /**
+ * Computes a monotonic-time deadline based on the provided timeout.
+ *
+ * <p>The returned value is expressed in the same time domain as
+ * {@link Utils#monotonicMs()} and is intended for comparisons against
+ * that value (for example, {@code now >= deadline}).
+ *
+ * <p>Timeout semantics:
+ * <ul>
+ * <li>{@code timeoutMillis < 0} or {@code timeoutMillis ==
Long.MAX_VALUE}
+ * — the deadline is unbounded and {@code Long.MAX_VALUE} is
returned.</li>
+ * <li>{@code timeoutMillis >= 0} — the deadline is computed as
+ * {@code Utils.monotonicMs() + timeoutMillis}.</li>
+ * </ul>
+ *
+ * @param timeoutMillis timeout in milliseconds controlling the retry
window
+ * @return a monotonic-time deadline in milliseconds, or {@code
Long.MAX_VALUE}
+ * if retries are unbounded
+ */
+ public static long monotonicMsAfter(long timeoutMillis) {
Review Comment:
Also I guess @denis-chudov means to place this method mainly to
`IgniteUtils` alongside with `monotonicMs`, just to be able to call this in
general without this raft's util class.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -207,29 +210,28 @@ RetryContext nextAttemptForUnavailablePeer(Peer
newTargetPeer, String shortReaso
}
TimeoutException createTimeoutException() {
- long ct = System.currentTimeMillis();
-
return new TimeoutException(format(
"Send with retry timed out [retryCount = {}, groupId = {},
traceId = {}, request = {}, originCommand = {},"
- + " retryReasons = {}, stopTime = {}, currentTime =
{}, startTime = {}, duration = {}].",
+ + " retryReasons = {}, stopTime = {}, currentTime =
{}, startTime = {}, duration = {}, currentWallTime = {}].",
retryCount,
groupId,
errorTraceId,
request.toStringForLightLogging(),
originDescription.get(),
retryReasons.toString(),
stopTime,
- ct,
+ Utils.monotonicMs(),
Review Comment:
Should we get the value once as it used to and reuse below?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
