You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "stefan-egli (via GitHub)" <gi...@apache.org> on 2024/01/29 15:59:54 UTC
[PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
stefan-egli opened a new pull request, #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288
…y atm
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1921925331
Trying to summarize the current options:
1. `static`
2. `DocumentStore.getRecoveryDelayMillis`
3. a new one that Rishabh suggested offline: `isRecoveryNeeded` requiring a 2nd parameter `recoveryDelayMillis` directly
4. `static` (like 1.) but add a loud disclaimer that this should be cleaned up - ie create a technical debt ticket to look into a more broader refactoring that also includes similar other cases such as the static `clock` for example.
opinions?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1470601490
##########
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:
Since this variable is a runtime time constant, I would make it final and if required would use reflection (or any other work around) to update this in tests OR better write test as such that I don't need to override in the first place.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1472558634
##########
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:
> To me it seems to be an anti-pattern or hacky way of implementing this change.
If it is considered anti-pattern or hacky that is of course not good.
I'll work on an alternative
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1938234187
My Vote in preferred order : 3 > 1
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "mbaedke (via GitHub)" <gi...@apache.org>.
mbaedke commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1938178323
Maybe I'm just stupid, but I fail to see why the use of a static field is an antipattern in this particular case.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "Joscorbe (via GitHub)" <gi...@apache.org>.
Joscorbe commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1933987123
As mentioned, I don't think the `static` approach to be so problematic. It's not the first place in Oak we use this approach and I don't really see a reason to have to change it in this specific PR.
A bit of refactor of entire Oak codebase is probably needed to comply with Java best practices, we can have a disclaimer or following ticket to do it generally.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1470601490
##########
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:
Since this variable is a compile time constant, I would make it final and if required would use reflection (or any other work around) to update this in tests OR better write test as such that I don't need to override in the first place.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1916917748
@rishabhdaim tests added [here](https://github.com/apache/jackrabbit-oak/pull/1288/commits/f1fcb179b7536ceb2b683751fdba25f9b281a4d4)
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1469983458
##########
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:
That would have been an option, but given I like to explicitly overwrite it in tests, I have now chosen the non final way. What's the advantage of having it final?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##########
@@ -520,6 +520,9 @@ public void handleLeaseFailure() {
if (isThrottlingEnabled(builder)) {
builder.setThrottlingStatsCollector(new ThrottlingStatsCollectorImpl(statisticsProvider));
}
+
+ // initialize the (global) recoveryDelayMillis
+ ClusterNodeInfo.setRecoveryDelayMillis(builder.getRecoveryDelayMillis());
Review Comment:
recoveryDelayMillis is static though?
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1471678539
##########
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:
To me it seems to be an anti-pattern or hacky way of implementing this change.
IMO our approach shouldn't be decided by no. of changes rather than correct way of implementing it.
But in this case if you think the no. of changes are too much of an effort vs the gain that we get from them, then may be we can go with static field approach as well. I will leave that upto you to decide.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1472745861
##########
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:
Created a draft of an alternative at https://github.com/apache/jackrabbit-oak/pull/1292
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1933923724
@Joscorbe , @rishabhdaim , @reschke , @mbaedke ,
I'd like to do a vote on the different options discussed in this OAK-10281 PR - namely:
Trying to summarize the current options:
1. https://github.com/apache/jackrabbit-oak/pull/1288 : `static`
2. https://github.com/apache/jackrabbit-oak/pull/1292 : `DocumentStore.getRecoveryDelayMillis`
3. https://github.com/apache/jackrabbit-oak/pull/1301 : a new one that Rishabh suggested offline: `isRecoveryNeeded` requiring a 2nd parameter `recoveryDelayMillis` directly
4. (similar to https://github.com/apache/jackrabbit-oak/pull/1288) : `static` (like 1.) but add a loud disclaimer that this should be cleaned up - ie create a technical debt ticket to look into a more broader refactoring that also includes similar other cases such as the static `clock` for example.
Please vote / comment, thx!
My vote : 1.
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli merged PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "stefan-egli (via GitHub)" <gi...@apache.org>.
stefan-egli commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1933909086
Created https://github.com/apache/jackrabbit-oak/pull/1301 as a draft for doing it via a parameter to `isRecoveryNeeded`
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#discussion_r1469944510
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreService.java:
##########
@@ -520,6 +520,9 @@ public void handleLeaseFailure() {
if (isThrottlingEnabled(builder)) {
builder.setThrottlingStatsCollector(new ThrottlingStatsCollectorImpl(statisticsProvider));
}
+
+ // initialize the (global) recoveryDelayMillis
+ ClusterNodeInfo.setRecoveryDelayMillis(builder.getRecoveryDelayMillis());
Review Comment:
I would initialize `recoveryDelayMillis` from the constructor.
##########
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:
I would make this field final and if needed would use reflection to override value in test cases.
See https://github.com/apache/jackrabbit-oak/blob/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java#L434
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10281 : osgi config variant of recovery delay - vs system propert… [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on PR #1288:
URL: https://github.com/apache/jackrabbit-oak/pull/1288#issuecomment-1916341838
I would add test cases in https://github.com/apache/jackrabbit-oak/blob/trunk/oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreServiceConfigurationTest.java
--
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-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org