You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2020/01/27 06:34:22 UTC
[GitHub] [curator] naude-r opened a new pull request #343: fix
ServiceCacheImpl thread leak
naude-r opened a new pull request #343: fix ServiceCacheImpl thread leak
URL: https://github.com/apache/curator/pull/343
- ServiceCacheImpl should allow the self-created executor service to be closed
- Allow an optional executor service for ServiceProvider
fix #CURATOR-557
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371471375
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceProviderBuilderImpl.java
##########
@@ -102,4 +105,18 @@
filters.add(filter);
return this;
}
+
Review comment:
Now that you added these 2 methods, you should update method `threadFactory(...)` to nullify `this.executorService`..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371451850
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceProviderBuilder.java
##########
@@ -52,6 +54,7 @@
* @param threadFactory factory to use
* @return this
*/
Review comment:
Please add to the Javadoc:
`* @deprecated use {@link #executorService(ExecutorService)} instead`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim merged pull request #343: [CURATOR-557]
ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim merged pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371471375
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceProviderBuilderImpl.java
##########
@@ -102,4 +105,18 @@
filters.add(filter);
return this;
}
+
Review comment:
Now that you added these 2 method, you should update method threadFactory(...) to nullify this.executorService..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371474832
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceProviderImpl.java
##########
@@ -43,13 +46,21 @@
private final ProviderStrategy<T> providerStrategy;
private final DownInstanceManager<T> downInstanceManager;
- public ServiceProviderImpl(ServiceDiscoveryImpl<T> discovery, String serviceName, ProviderStrategy<T> providerStrategy, ThreadFactory threadFactory, List<InstanceFilter<T>> filters, DownInstancePolicy downInstancePolicy)
+ public ServiceProviderImpl(ServiceDiscoveryImpl<T> discovery, String serviceName, ProviderStrategy<T> providerStrategy, ThreadFactory threadFactory, CloseableExecutorService executorService, List<InstanceFilter<T>> filters, DownInstancePolicy downInstancePolicy)
Review comment:
This constructor is `public` (can be used directly, skipping `ServiceProviderBuilder`), and now it accepts both `ThreadFactory` and `CloseableExecutorService`, which can be confusing.
I suggest to make this constructor package-private and add 2 `public` constructors: one that accepts all arguments as this one but only `ThreadFactory` (no `CloseableExecutorService`) and second that accepts all arguments as this one but only `CloseableExecutorService` (no `ThreadFactory`). Both will delegate to this constructor (with `null` as the missing argument).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on issue #343: [CURATOR-557]
ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on issue #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#issuecomment-579978051
@naude-r yes, please squash the commits before your next push. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371488236
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceCacheBuilder.java
##########
@@ -46,6 +46,7 @@
* @param threadFactory factory
* @return this
*/
Review comment:
Please add to the Javadoc:
`* @deprecated use {@link #executorService(ExecutorService)} instead`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on issue #343: [CURATOR-557]
ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on issue #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#issuecomment-580093604
@shayshim review comments addressed and commits squashed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371626536
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceProviderBuilder.java
##########
@@ -71,4 +74,23 @@
* @return this
*/
public ServiceProviderBuilder<T> additionalFilter(InstanceFilter<T> filter);
+
+ /**
+ * Optional ExecutorService to use for the cache's background thread. The specified ExecutorService
+ * will be wrapped in a CloseableExecutorService and overrides any prior ThreadFactory or ExecutorService
Review comment:
done for both.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on issue #343: [CURATOR-557]
ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on issue #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#issuecomment-579099724
@shayshim changes made. have not squashed the commit. mostly so that review comments stay intact. can do this if required.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371626088
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceProviderBuilder.java
##########
@@ -52,6 +54,7 @@
* @param threadFactory factory to use
* @return this
*/
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371626468
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceProviderImpl.java
##########
@@ -43,13 +46,21 @@
private final ProviderStrategy<T> providerStrategy;
private final DownInstanceManager<T> downInstanceManager;
- public ServiceProviderImpl(ServiceDiscoveryImpl<T> discovery, String serviceName, ProviderStrategy<T> providerStrategy, ThreadFactory threadFactory, List<InstanceFilter<T>> filters, DownInstancePolicy downInstancePolicy)
+ public ServiceProviderImpl(ServiceDiscoveryImpl<T> discovery, String serviceName, ProviderStrategy<T> providerStrategy, ThreadFactory threadFactory, CloseableExecutorService executorService, List<InstanceFilter<T>> filters, DownInstancePolicy downInstancePolicy)
Review comment:
done. constructor was made protected so that:
* it can be extended in future
* ServiceProviderBuilderImpl can access 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371491756
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceProviderBuilder.java
##########
@@ -71,4 +74,23 @@
* @return this
*/
public ServiceProviderBuilder<T> additionalFilter(InstanceFilter<T> filter);
+
+ /**
+ * Optional ExecutorService to use for the cache's background thread. The specified ExecutorService
+ * will be wrapped in a CloseableExecutorService and overrides any prior ThreadFactory or ExecutorService
Review comment:
Should be `... and overrides any prior ThreadFactory or CloseableExecutorService` (instead of `ExecutorService`), no?
Same question for next method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371626495
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceCacheBuilder.java
##########
@@ -46,6 +46,7 @@
* @param threadFactory factory
* @return this
*/
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371626160
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceProviderBuilderImpl.java
##########
@@ -102,4 +105,18 @@
filters.add(filter);
return this;
}
+
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r372640591
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceCacheBuilder.java
##########
@@ -46,6 +46,7 @@
* @param threadFactory factory
* @return this
*/
Review comment:
Sorry if I wasn't clear, but I didn't mean to drop the `@Deprecated`. `@Deprecated` and `@deprecated` should be used together. Please refer to:
https://docs.oracle.com/javase/8/docs/technotes/guides/javadoc/deprecation/deprecation.html
Their conclusion is "Thus, using the `@Deprecated` annotation to generate warnings is more portable that relying on the `@deprecated` Javadoc tag."
Same for `ServiceProviderBuilder.threadFactory(...)`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] naude-r commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
naude-r commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r372768504
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceCacheBuilder.java
##########
@@ -46,6 +46,7 @@
* @param threadFactory factory
* @return this
*/
Review comment:
done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r371467820
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceProviderBuilder.java
##########
@@ -52,6 +54,7 @@
* @param threadFactory factory to use
Review comment:
Please add to Javadoc something like:
`The specified ThreadFactory overrides any prior ThreadFactory or ClosableExecutorService set on the ServiceProviderBuilder`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] shayshim commented on a change in pull request #343:
[CURATOR-557] ServiceCacheImpl does not close ExecutorService
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #343: [CURATOR-557] ServiceCacheImpl does not close ExecutorService
URL: https://github.com/apache/curator/pull/343#discussion_r372648860
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceCacheBuilder.java
##########
@@ -46,6 +46,7 @@
* @param threadFactory factory
* @return this
*/
Review comment:
Also, it turns out that deprecation of interface method doesn't affect overriding methods, as said in the same link:
"It is possible for a non-deprecated property to hide or override a deprecated one, which removes deprecation. As developer of an API, it is your responsibility to deprecate overrides of a deprecated method, if in fact they should be deprecated."
Could you deprecate also the two `threadFactory(...)` overriding methods?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services