You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by swankjesse <gi...@git.apache.org> on 2015/01/02 17:55:50 UTC
[GitHub] curator pull request: Use Curator in thread names.
GitHub user swankjesse opened a pull request:
https://github.com/apache/curator/pull/60
Use Curator in thread names.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/swankjesse/curator jwilson_0102_name_threads
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/60.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #60
----
commit ea155f0ffafb6f9795ba83f27eca98738490d75d
Author: Jesse Wilson <jw...@squareup.com>
Date: 2015-01-02T16:55:12Z
Use Curator in thread names.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/60#discussion_r22500645
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorTempFrameworkImpl.java ---
@@ -98,7 +99,15 @@ private synchronized void openConnectionIfNeeded() throws Exception
if ( cleanup == null )
{
ThreadFactory threadFactory = factory.getThreadFactory();
- cleanup = (threadFactory != null) ? Executors.newScheduledThreadPool(1, threadFactory) : Executors.newScheduledThreadPool(1);
+
--- End diff --
But @Randgalt can you verify that this should be non-daemon? Just wondering if it's a latent bug here that this will tie up process exit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/60#issuecomment-68748082
+1 to @Randgalt suggestion
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/60#issuecomment-68658796
It would be a lot simpler to make the change in ThreadUtils, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by swankjesse <gi...@git.apache.org>.
Github user swankjesse commented on the pull request:
https://github.com/apache/curator/pull/60#issuecomment-68542197
https://issues.apache.org/jira/browse/CURATOR-177
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/60
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/60#discussion_r22476977
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorTempFrameworkImpl.java ---
@@ -98,7 +99,15 @@ private synchronized void openConnectionIfNeeded() throws Exception
if ( cleanup == null )
{
ThreadFactory threadFactory = factory.getThreadFactory();
- cleanup = (threadFactory != null) ? Executors.newScheduledThreadPool(1, threadFactory) : Executors.newScheduledThreadPool(1);
+
--- End diff --
Is there a ThreadUtils call that could replace?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/60#issuecomment-68563856
LGTM!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by swankjesse <gi...@git.apache.org>.
Github user swankjesse commented on the pull request:
https://github.com/apache/curator/pull/60#issuecomment-68771906
PTAL. I made the change in `ThreadUtils` instead, and fixed callsites.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by swankjesse <gi...@git.apache.org>.
Github user swankjesse commented on a diff in the pull request:
https://github.com/apache/curator/pull/60#discussion_r22486680
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorTempFrameworkImpl.java ---
@@ -98,7 +99,15 @@ private synchronized void openConnectionIfNeeded() throws Exception
if ( cleanup == null )
{
ThreadFactory threadFactory = factory.getThreadFactory();
- cleanup = (threadFactory != null) ? Executors.newScheduledThreadPool(1, threadFactory) : Executors.newScheduledThreadPool(1);
+
--- End diff --
No. The existing ThreadUtils factory only builds daemon threads.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: Use Curator in thread names.
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/60#discussion_r22500625
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorTempFrameworkImpl.java ---
@@ -98,7 +99,15 @@ private synchronized void openConnectionIfNeeded() throws Exception
if ( cleanup == null )
{
ThreadFactory threadFactory = factory.getThreadFactory();
- cleanup = (threadFactory != null) ? Executors.newScheduledThreadPool(1, threadFactory) : Executors.newScheduledThreadPool(1);
+
--- End diff --
Maybe we want a ThreadUtils.newNonDaemonThreadFactory() to replace this?
But LGTM either way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---