You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2023/06/26 14:40:05 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
dlmarion opened a new pull request, #3542:
URL: https://github.com/apache/accumulo/pull/3542
TabletGroupWatcher prior to this change was hardcoded to wait for 60s between loops looking for tablets that needed some type of maintenance. This property allows the user to configure that potentially making the Manager more responsive to tablets being unassigned or unbalanced.
Fixes #3541
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #3542:
URL: https://github.com/apache/accumulo/pull/3542
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1243615580
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -449,6 +448,13 @@ public static void main(String[] args) throws Exception {
log.info("SASL is not enabled, delegation tokens will not be available");
delegationTokensAvailable = false;
}
+ this.waitTimeBetweenScans =
+ aconf.getTimeInMillis(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL);
Review Comment:
I think I can do this without the supplier. TabletGroupWatcher calls `manager.getWaitTImeBetweenScans()`. That method can just get the current value of the property from the configuration and return it.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#issuecomment-1609477220
How does a user know what a "correct" or "better" value would be over the default for tuning? Is there a log statement (or a metric) that reports how long the the triggered action takes to run?
If the processing time is longer than what the user selects for the wait time - could it lead to an ever growing backlog of tasks - or could it lead to resource starvation because it never gets to pause and just runs continuously?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1245747017
##########
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:
Created a separate property in ff6923e
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1245747745
##########
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:
Created a local variable at the top of the loop so that the value for the property is the same for the entire iteration, in ff6923e
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1242789779
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1101,7 +1107,7 @@ public void run() {
log.error("Error initializing metrics, metrics will not be emitted.", e1);
}
- recoveryManager = new RecoveryManager(this, TIME_TO_CACHE_RECOVERY_WAL_EXISTENCE);
+ recoveryManager = new RecoveryManager(this, timeToCacheRecoveryWalExistence);
Review Comment:
This should pass a LongSupplier to compute the value dynamically from the current value of the configurable interval time.
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -197,7 +197,7 @@ public void run() {
}
if (currentTServers.isEmpty()) {
- eventListener.waitForEvents(Manager.TIME_TO_WAIT_BETWEEN_SCANS);
+ eventListener.waitForEvents(manager.getWaitTimeBetweenScans());
Review Comment:
Using the Supplier idea I suggested, the value can be obtained once at the top of this loop, and reused for the rest of the loop iteration, using a final local variable, so it doesn't change in the middle of the iteration.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -449,6 +448,13 @@ public static void main(String[] args) throws Exception {
log.info("SASL is not enabled, delegation tokens will not be available");
delegationTokensAvailable = false;
}
+ this.waitTimeBetweenScans =
+ aconf.getTimeInMillis(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL);
Review Comment:
Computing it once at the time of the Manager startup makes this a fixed property, only changeable at restarts. You can do that, of course, but should add it to the `Property.fixedProperties`. However, a better idea is to have the field be a LongSupplier, and compute it from the config dynamically. You can use Guava's `Suppliers.memoize` to create the supplier. You can configure the memoizing supplier to cache the value for a certain amount of time, if you don't want it to do the lookup from the config too frequently.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1243610488
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1101,7 +1107,7 @@ public void run() {
log.error("Error initializing metrics, metrics will not be emitted.", e1);
}
- recoveryManager = new RecoveryManager(this, TIME_TO_CACHE_RECOVERY_WAL_EXISTENCE);
+ recoveryManager = new RecoveryManager(this, timeToCacheRecoveryWalExistence);
Review Comment:
I'm not sure that makes sense here. This is done during Manager startup before clients can connect to it. The RecoveryManager is created and the value of `timeToCacheRecoveryWalExistence` is used in the RecoveryManager constructor to set the expiration time on the Caffeine CacheBuilder. This is only called once, not in a loop, and I don't think the Caffeine cache can be updated after construction.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#issuecomment-1609749764
> Is there a log statement (or a metric) that reports how long the the triggered action takes to run?
A [message](https://github.com/apache/accumulo/blob/2.1/server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java#L349) is printed to the log that it's waiting for some amount of time. This property just changes the the amount of wait time before the next check starts. A user could determine the time delta between these log statements to determine how long one iteration of the loop takes.
> If the processing time is longer than what the user selects for the wait time - could it lead to an ever growing backlog of tasks
Yes, but that could happen right now too.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1245748182
##########
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:
Updated property description in ff6923e
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1243632537
##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -197,7 +197,7 @@ public void run() {
}
if (currentTServers.isEmpty()) {
- eventListener.waitForEvents(Manager.TIME_TO_WAIT_BETWEEN_SCANS);
+ eventListener.waitForEvents(manager.getWaitTimeBetweenScans());
Review Comment:
baea8d7 provides the TabletGroupWatcher with the current value of the property.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -449,6 +448,13 @@ public static void main(String[] args) throws Exception {
log.info("SASL is not enabled, delegation tokens will not be available");
delegationTokensAvailable = false;
}
+ this.waitTimeBetweenScans =
+ aconf.getTimeInMillis(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL);
Review Comment:
baea8d7 provides the TabletGroupWatcher with the current value of the property.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3542: Backported MANAGER_TABLET_GROUP_WATCHER_INTERVAL property
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3542:
URL: https://github.com/apache/accumulo/pull/3542#discussion_r1245765894
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -449,6 +447,12 @@ public static void main(String[] args) throws Exception {
log.info("SASL is not enabled, delegation tokens will not be available");
delegationTokensAvailable = false;
}
+ this.timeToCacheRecoveryWalExistence =
+ aconf.getTimeInMillis(Property.MANAGER_RECOVERY_WAL_EXISTENCE_CACHE_TIME);
+ }
+
+ public long getWaitTimeBetweenScans() {
+ return this.getConfiguration().getTimeInMillis(Property.MANAGER_TABLET_GROUP_WATCHER_INTERVAL);
Review Comment:
Since this is now only used once, and the implementation is trivial, this should now be inline'd, rather than add a new API method to Manager.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -311,6 +311,13 @@ 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 identify tablets that need to be assigned, un-assigned, migrated, etc.",
+ "2.1.2"),
+ MANAGER_RECOVERY_WAL_EXISTENCE_CACHE_TIME("manager.recovery.wal.cache.time", "15s",
+ PropertyType.TIMEDURATION,
+ "Amount of time that the existence of recovery write-ahead logs is cached.", "2.1.2"),
Review Comment:
This should be grouped adjacent to the other `manager.recovery.*` property.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org