stefan-egli commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1471238975
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/ClusterNodeInfo.java:
##########
@@ -229,13 +229,14 @@ static RecoverLockState fromString(String state) {
/** OAK-3399 : max number of times we're doing a 1sec retry loop just
before declaring lease failure **/
private static final int MAX_RETRY_SLEEPS_BEFORE_LEASE_FAILURE = 5;
- /** OAK-10281 : seconds to delay a recovery after a lease timeout */
- private static final int DEFAULT_RECOVERY_DELAY_SECS =
SystemPropertySupplier.create("oak.documentMK.recoveryDelaySecs", 0)
- .loggingTo(LOG).validateWith(value -> value >= 0)
- .formatSetMessage((name, value) -> String.format("recovery delay
set to (secs): %ss (using system property %s)", name, value)).get();
- private static final long DEFAULT_RECOVERY_DELAY_MILLIS = 1000L *
(long)DEFAULT_RECOVERY_DELAY_SECS;
+ /** OAK-10281 : default millis to delay a recovery after a lease timeout */
+ static final long DEFAULT_RECOVERY_DELAY_MILLIS = 0;
- // kept non-final for testing purpose
+ /**
+ * Actual millis to delay a recovery after a lease timeout.
+ * <p>
+ * Initialized by DocumentNodeStore constructor.
+ */
static long recoveryDelayMillis = DEFAULT_RECOVERY_DELAY_MILLIS;
Review Comment:
Upon revisit, non-final actually doesn't work as this is modified for both
test and non-test case. The latter being the main use case:
[DocumentNodeStoreService.configureBuilder](https://github.com/apache/jackrabbit-oak/blob/OAK-10281-2/oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java#L528).
Replacing this static field altogether, which would probably be preferred,
however isn't possible without a larger change : it would somehow have to be
passed to ClusterNodeInfoDocument for it to be available at isRecoveryNeeded
invocation time. That could eg be done if it was available at constructor time.
From there it goes on to perhaps being available in DocumentStore. But all of
that doesn't make it simpler but rather adds more clutter... which is the
reason for the static approach. Not the nicest, but somewhat nicer than adding
it to several additional objects.
If we'd go the static route, then I'd argue it can't be final. And going the
non-static route seems heavy.
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
