ctubbsii commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1244248443
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -311,6 +311,9 @@ public enum Property {
"The balancer class that accumulo will use to make tablet assignment and
"
+ "migration decisions.",
"1.3.5"),
+ MANAGER_TABLET_GROUP_WATCHER_INTERVAL("manager.tablet.watcher.interval",
"60s",
+ PropertyType.TIMEDURATION,
+ "Time to wait between scanning tablet states to determine migrations,
etc.", "2.1.2"),
Review Comment:
I'm not sure `etc.` is doing much to help the reader understand the category
of things this affects. The migrations is only one example. If there were a
second example in the list, then `etc.` could make sense, because one might be
able to infer a category of tasks from two examples. If there's no second
example to add to the list, you could just phrase it as "states to determine
outstanding tasks, such as migrations."
```suggestion
"Time to wait between scanning tablet states to identify outstanding
tasks to perform, such as migrations.", "2.1.2"),
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -347,8 +347,8 @@ public void run() {
}
if
(manager.tserverSet.getCurrentServers().equals(currentTServers.keySet())) {
Manager.log.debug(String.format("[%s] sleeping for %.2f seconds",
store.name(),
- Manager.TIME_TO_WAIT_BETWEEN_SCANS / 1000.));
- eventListener.waitForEvents(Manager.TIME_TO_WAIT_BETWEEN_SCANS);
+ manager.getWaitTimeBetweenScans() / 1000.));
+ eventListener.waitForEvents(manager.getWaitTimeBetweenScans());
Review Comment:
I still think you need to use a local variable in here, so the log message
isn't logging a different value than what's actually being used, which makes
debugging a pain.
And, this, as well as the occurrence above are still in the same `while
(manager.stillManager())` loop. It would be weird if the value changed from one
operation to another in that same loop. They should share the same local value.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -449,6 +447,13 @@ public static void main(String[] args) throws Exception {
log.info("SASL is not enabled, delegation tokens will not be available");
delegationTokensAvailable = false;
}
+ long waitTimeBetweenScans =
+ aconf.getTimeInMillis(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL);
+ this.timeToCacheRecoveryWalExistence = waitTimeBetweenScans / 4;
+ }
+
+ public long getWaitTimeBetweenScans() {
+ return
this.getConfiguration().getTimeInMillis(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL);
Review Comment:
The `getWaitTimeBetweenScans()` method returns the current value, but
`this.timeToCacheRecoveryWalExistence` is still based on, and fixed to, the
initial value. It doesn't seem like a good idea to have a property be
runtime-mutable, but also be the basis for a runtime-immutable characteristic,
at the same time. Should these be separate properties, one that is a
fixedProperty and one that is mutable at runtime?
--
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]
