You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/09/21 19:59:27 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
dlmarion opened a new pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282
Commit e62ace6a9d37572d95999f5412c6148efbba50b9 removed SimpleTimer in favor of
a new ThreadPools class that centralized the creation of thread pools. However, I
missed the fact that SimpleTimer used a shared thread pool. This change reintroduces
a shared generic ScheduledThreadPoolExecutor that can be obtained by calling
ServerContext.getSharedGenericScheduledExecutorService().
Closes #2280
--
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 #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#issuecomment-926012504
I thought about this as well when I was making this change. I think I was looking at the CompactionCoordinator and noticed that it put for tasks in the thread pool. I was considering that maybe we should change the default value of GENERAL_SIMPLETIMER_THREADPOOL_SIZE from 1 to something else. I'm not sure that there is a penalty for idle threads. Maybe the default should be 4 ?
--
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 edited a comment on pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion edited a comment on pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#issuecomment-926012504
I thought about this as well when I was making this change. I think I was looking at the CompactionCoordinator and noticed that it put four tasks in the thread pool. I was considering that maybe we should change the default value of GENERAL_SIMPLETIMER_THREADPOOL_SIZE from 1 to something else. I'm not sure that there is a penalty for idle threads. Maybe the default should be 4 ?
--
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 change in pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#discussion_r714013565
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -189,6 +189,10 @@ public static ThreadPoolExecutor createThreadPool(int coreThreads, int maxThread
return result;
}
+ /*
+ * If you need the server-side shared ScheduledThreadPoolExecutor, then use
+ * ServerContext.getSharedGenericScheduledExecutorService()
+ */
public static ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
Review comment:
Not all calls were replaced w/ the new context method. There are places in the code where the new method `ThreadPools.createGeneralScheduledExecutorService` did not replace the use of SimpleTimer. For example, in the [TabletServerBatchWriter](ThreadPools.createGeneralScheduledExecutorService). For this change I went back through diff of commit e62ace6a9d37572d95999f5412c6148efbba50b9 and used the new context method in places where SimpleTimer was being used.
--
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 change in pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#discussion_r714079225
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -189,6 +189,10 @@ public static ThreadPoolExecutor createThreadPool(int coreThreads, int maxThread
return result;
}
+ /*
+ * If you need the server-side shared ScheduledThreadPoolExecutor, then use
+ * ServerContext.getSharedGenericScheduledExecutorService()
Review comment:
```suggestion
* ServerContext.getScheduledExecutor()
```
--
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 pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#issuecomment-926000446
I'm not sure it was necessary to create a shared thread pool for all SimpleTimer use cases. In fact, that was probably likely to create problems for us, with some operations blocking others, due to overuse of the off-the-shelf SimpleTimer class. I did notice this difference when I reviewed your original ThreadPools changes, but considered it an improvement, as it decoupled unnecessarily intertwined operations. I think going back to a single thread pool for these might actually be a regression. Do we really need a SimpleTimer-like object with a shared thread pool? I feel like it's going to bit us in future to have something off-the-shelf like that. However, I do like the explicit naming to at least make it clear what the intended behavior is.
--
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 #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#issuecomment-926015613
Another option would be to make the generic scheduled thread pool resize itself in all cases like we are doing [here](https://github.com/apache/accumulo/pull/2282/files#diff-082a1c54b559610df4a2cdcb0bc33144ce21696d487a2af5c00b355ebcf975cbR139), but instead of using the current value of the property, use the # of tasks in the queue.
--
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] keith-turner commented on a change in pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#discussion_r713395127
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -189,6 +189,10 @@ public static ThreadPoolExecutor createThreadPool(int coreThreads, int maxThread
return result;
}
+ /*
+ * If you need the server-side shared ScheduledThreadPoolExecutor, then use
+ * ServerContext.getSharedGenericScheduledExecutorService()
+ */
public static ScheduledThreadPoolExecutor
createGeneralScheduledExecutorService(AccumuloConfiguration conf) {
Review comment:
Were all calls this replaced w/ the new context method or are there still some direct calls to it?
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -433,4 +435,13 @@ private void monitorSwappiness(AccumuloConfiguration config) {
}
}, 1000, 10 * 60 * 1000, TimeUnit.MILLISECONDS);
}
+
+ public ScheduledThreadPoolExecutor getSharedGenericScheduledExecutorService() {
Review comment:
Can sync to avoid race condition of multiple thread creating when its null. Also I think a shorter name would be nice, withe the fact that its shared moved to javadoc.
```suggestion
/**
* return a shared scheduled executor
*/
public synchronized ScheduledThreadPoolExecutor getScheduledExecutor() {
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/session/SessionManager.java
##########
@@ -65,10 +66,10 @@
private final Long expiredSessionMarker = (long) -1;
private final AccumuloConfiguration aconf;
- public SessionManager(AccumuloConfiguration conf) {
- aconf = conf;
- maxUpdateIdle = conf.getTimeInMillis(Property.TSERV_UPDATE_SESSION_MAXIDLE);
- maxIdle = conf.getTimeInMillis(Property.TSERV_SESSION_MAXIDLE);
+ public SessionManager(ServerContext context) {
+ this.aconf = context.getConfiguration();
Review comment:
This comment is not about this particular change, but more about all of the changes that switched from passing config to passing context. Did you verify that all of these types of changes are still using the same config type? For example if it used to be the case that table or site config were passed in, is it getting the same thing from the context?
--
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 pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#issuecomment-926020254
> Another option would be to make the generic scheduled thread pool resize itself in all cases like we are doing [here](https://github.com/apache/accumulo/pull/2282/files#diff-082a1c54b559610df4a2cdcb0bc33144ce21696d487a2af5c00b355ebcf975cbR139), but instead of using the current value of the property, use the # of tasks in the queue.
The point of SimpleTimer was to be a trivial implementation for quick uses. I don't think it's worth making something so feature heavy for the use cases SimpleTimer was written for. Changing the default size of the thread pool to a small number greater than 1 could help avoid blocking simple operations. However, it still has the same basic problem that it makes it easy for use to misuse it by not considering the impact of contention with other uses... but those problems might be less obvious and harder to detect if the contention is less consistent because we increased the size. It might be best to simply discourage the use of a "simple" timer with shared resources.
--
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 #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282
--
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 change in pull request #2282: Recreated the SimpleTimer functionality with the new ThreadPools class
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2282:
URL: https://github.com/apache/accumulo/pull/2282#discussion_r714049059
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/session/SessionManager.java
##########
@@ -65,10 +66,10 @@
private final Long expiredSessionMarker = (long) -1;
private final AccumuloConfiguration aconf;
- public SessionManager(AccumuloConfiguration conf) {
- aconf = conf;
- maxUpdateIdle = conf.getTimeInMillis(Property.TSERV_UPDATE_SESSION_MAXIDLE);
- maxIdle = conf.getTimeInMillis(Property.TSERV_SESSION_MAXIDLE);
+ public SessionManager(ServerContext context) {
+ this.aconf = context.getConfiguration();
Review comment:
So, I think this specific instance is ok. In most cases, the AccumuloConfiguration being passes is from `context.getConfiguration`. I did revert a couple of cases where I could not be sure, DistributedWorkQueue and AssignmentWatcher.
--
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