ctubbsii commented on code in PR #3161:
URL: https://github.com/apache/accumulo/pull/3161#discussion_r1073088711
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -348,6 +348,16 @@ public enum Property {
+ "indefinitely. Default is 0 to block indefinitely. Only valid when
tserver available "
+ "threshold is set greater than 0. Added with version 1.10",
"1.10.0"),
+ MANAGER_LOW_MEM_DETECTOR_ACTIVE("manager.low.mem.detector.active", "false",
PropertyType.BOOLEAN,
+ "By default the LowMemoryDetector is passive, it just logs that memory
is low. Setting this to true"
+ + " may change the behavior of server components that check the
LowMemoryDetector state",
+ "3.0.0"),
+ MANAGER_LOW_MEM_DETECTOR_INTERVAL("manager.low.mem.detector.interval", "5s",
+ PropertyType.TIMEDURATION, "The number of milliseconds between low
memory checks", "3.0.0"),
+ MANAGER_LOW_MEM_DETECTOR_THRESHOLD("manager.low.mem.detector.treshold",
"0.05",
+ PropertyType.FRACTION,
+ "The LowMemoryDetector will report when free memory drops below this
percentage of total memory",
Review Comment:
If this were a general property, one could still deploy different servers
with different configs to achieve the same thing, without the redundancy. The
main advantage of having different keys for different servers is that the
configuration for different servers can be stored in the same place. But, I
don't think think that benefit is worth the verbosity here, especially since
memory management like this is likely to only matter to a very niche situation
where one has a sub-optimal JVM GC configuration and user behavior.
I think we can just do `general.low.mem.detector.{interval,threshold}`.
As for the `active` flag, I'm wondering if we need that at all, or if we
want to just have the server protect itself automatically in those situations.
I'm not sure what circumstances we'd want to leave it passive if we're likely
to die of OOM. Alternatively, it could be made a scan / iterator option, rather
than a server-level option. So, rather than have an "active/passive" mode on
the server as a whole, you'd have a "respect/ignore low memory" mode on a scan.
It would still be a server configuration property, but it might be more
intuitive to think about it and configure it that way.
Also, there's a typo in the threshold key here (you have it written as `tre`
instead of `thre`).. didn't check everywhere.
--
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]
