Mmuzaf commented on code in PR #2046:
URL: https://github.com/apache/cassandra/pull/2046#discussion_r1170095535
##########
src/java/org/apache/cassandra/gms/FailureDetector.java:
##########
@@ -55,21 +58,19 @@ public class FailureDetector implements IFailureDetector,
FailureDetectorMBean
private static final int SAMPLE_SIZE = 1000;
protected static final long INITIAL_VALUE_NANOS =
TimeUnit.NANOSECONDS.convert(getInitialValue(), TimeUnit.MILLISECONDS);
private static final int DEBUG_PERCENTAGE = 80; // if the phi is larger
than this percentage of the max, log a debug message
- private static final long DEFAULT_MAX_PAUSE = 5000L * 1000000L; // 5
seconds
private static final long MAX_LOCAL_PAUSE_IN_NANOS = getMaxLocalPause();
private long lastInterpret = preciseTime.now();
private long lastPause = 0L;
private static long getMaxLocalPause()
{
- if (System.getProperty("cassandra.max_local_pause_in_ms") != null)
- {
- long pause =
Long.parseLong(System.getProperty("cassandra.max_local_pause_in_ms"));
- logger.warn("Overriding max local pause time to {}ms", pause);
- return pause * 1000000L;
- }
- else
- return DEFAULT_MAX_PAUSE;
+ long pause = MAX_LOCAL_PAUSE_IN_MS.getLong();
+
+ if
(!String.valueOf(pause).equals(MAX_LOCAL_PAUSE_IN_MS.getDefaultValue()))
Review Comment:
Actually, we have plenty of options here and I have checked all of them
locally. My guess is we can leave it 'as is' for now for the following reasons:
- If we had an `isDefault` method it would only be used for 2 cases, I have
checked all usages of the `getInt()`, `getLong()`, `getString()` methods and
found only two cases that fit this usage pattern;
- The `isDefault` method cannot be used for overridden values at runtime
such as `public int getInt(int overrideDefaultValue)`, because in this case the
`defaultVal` is actually `null`, so the method itself doesn't help us much to
compare given overridden default at runtime;
- For the `getString(String default, boolean log)` and
`MAX_LOCAL_PAUSE_IN_MS("cassandra.max_local_pause_in_ms", "5000", true)`
methods, they don't seem to fit our requirements either, as they mix in the
properties logging in the API `CassandraRelevantProperties` design;
- I have also thought about the following API `public int
getInt(Consumer<Integer> fallback)`, so we will be able to do some action based
on the current or default value passed to the consumer, but this will
significantly increase the API access to system properties. I haven't found any
good examples to handle this in popular configuration frameworks (e.g.
`Lightbend`, `commons-configuration`).
In conclusion, as we only have two places with such a pattern, we can leave
it as it is and keep the API simple. wdyt?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
