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 2022/01/26 14:45:01 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2432: Modified ThreadPools to conditionally register metrics
dlmarion opened a new pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432
When ThreadPools calls ExecutorServiceMetrics, it creates Meters
that are registered with the MeterRegistry. However, there is no
easy way to remove the Meters from the MeterRegistry when the
ThreadPool is shut down. Modified the ThreadPools methods to accept
a boolean value for registering metrics for the newly created
ThreadPool. Modified classes that use ThreadPools class to only
pass true for this boolean value when the ThreadPool is long lived.
Closes #2423
--
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 #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#discussion_r792780757
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -104,81 +104,86 @@ public static void resizePool(final ThreadPoolExecutor pool, final AccumuloConfi
*/
@SuppressWarnings("deprecation")
public static ExecutorService createExecutorService(final AccumuloConfiguration conf,
- final Property p) {
+ final Property p, boolean emitThreadPoolMetrics) {
Review comment:
Could add some javadoc for this param like the following
```java
@param emitThreadPoolMetrics When set to true will emit metrics and register the metrics in a static registry. After the thread pool is deleted, there will still be metrics objects related to it in the static registry. There is no way to clean these left over objects up therefore its recommened that this option only be set true for long lived thread pools. Creating lots of short lived thread pools and registering them can lead to out of memory errors over long time periods.
```
--
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 #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#issuecomment-1023167707
> Boolean parameters can make code harder to read. Instead of boolean parameters, you could have a separate method name, like .action() and .actionWithMetrics(), or you could use a builder, like builder.withMetrics().build(), or you could have enums for flags, like .action(EnumSet.of(WITH_METRICS, OTHER_FLAG))
I have no issue with modifying ThreadPools to use a builder pattern, but I think that should be done in a different issue. IDEs make it pretty easy to see the formal parameter name for the method and the formal parameter name is descriptive in this case.
> Another option would be to replace the last boolean argument in the method with a new enum type. The single boolean here is not too bad, an enum would be more readable. We have had methods in the past w/ multiple boolean arguments and that was awful to read.
I have no issue with modifying this PR to use an enum for the boolean parameter, but I'm not sure that it's necessary if there is going to be a follow-on issue created to modify the class to use a builder pattern.
--
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 #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#discussion_r792813461
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -230,7 +230,7 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
- Property.MANAGER_FATE_THREADPOOL_SIZE);
+ Property.MANAGER_FATE_THREADPOOL_SIZE, false);
Review comment:
Addressed in 1a7f098
--
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 #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#discussion_r792783837
##########
File path: core/src/main/java/org/apache/accumulo/fate/Fate.java
##########
@@ -230,7 +230,7 @@ public Fate(T environment, TStore<T> store, Function<Repo<T>,String> toLogStrFun
*/
public void startTransactionRunners(AccumuloConfiguration conf) {
final ThreadPoolExecutor pool = (ThreadPoolExecutor) ThreadPools.createExecutorService(conf,
- Property.MANAGER_FATE_THREADPOOL_SIZE);
+ Property.MANAGER_FATE_THREADPOOL_SIZE, false);
Review comment:
May be ok to set this one to true. This method may only be called once by the manager.
--
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 pull request #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#issuecomment-1023360594
> IDEs make it pretty easy to see the formal parameter name for the method and the formal parameter name is descriptive in this case.
That is a good point. That does dramatically increase readability. However when reviewing a PR on GH, you dont have that nice feature. But thinking back to my review that was not a problem at all. The review was very easy for me to review because I knew what the single boolean did. The more difficult part of the review was that I had to look at code outside the PR sometimes to see how code was called externally, which had nothing to do with the boolean. More than one boolean would certainly have given me a headache during review though.
> I'm not sure that it's necessary if there is going to be a follow-on issue created to modify the class to use a builder pattern.
That makes sense to me.
--
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 pull request #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#issuecomment-1022672607
> Boolean parameters can make code harder to read. Instead of boolean parameters, you could have a separate method name, like .action() and .actionWithMetrics(), or you could use a builder, like builder.withMetrics().build(), or you could have enums for flags, like .action(EnumSet.of(WITH_METRICS, OTHER_FLAG))
Another option would be to replace the last boolean argument in the method with a new enum type. The single boolean here is not too bad, an enum would be more readable. We have had methods in the past w/ multiple boolean arguments and that was awful to read.
--
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 #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432#discussion_r792813662
##########
File path: core/src/main/java/org/apache/accumulo/core/util/threads/ThreadPools.java
##########
@@ -104,81 +104,86 @@ public static void resizePool(final ThreadPoolExecutor pool, final AccumuloConfi
*/
@SuppressWarnings("deprecation")
public static ExecutorService createExecutorService(final AccumuloConfiguration conf,
- final Property p) {
+ final Property p, boolean emitThreadPoolMetrics) {
Review comment:
Addressed in 1a7f098
--
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 #2432: Modified ThreadPools to conditionally register metrics
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2432:
URL: https://github.com/apache/accumulo/pull/2432
--
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