You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by dragonsinth <gi...@git.apache.org> on 2014/08/22 22:01:28 UTC
[GitHub] curator pull request: Builder to configure advanced TreeCache opti...
GitHub user dragonsinth opened a pull request:
https://github.com/apache/curator/pull/41
Builder to configure advanced TreeCache options.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dragonsinth/curator CURATOR-144
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/41.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 #41
----
commit d914e501df10d13d98b4f27bcce20d7c693df397
Author: Scott Blum <sc...@squareup.com>
Date: 2014-08-22T20:00:20Z
Builder to configure advanced TreeCache options.
----
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16851967
--- Diff: curator-client/src/main/java/org/apache/curator/utils/CloseableExecutorService.java ---
@@ -116,7 +116,7 @@ public CloseableExecutorService(ExecutorService executorService)
*/
public CloseableExecutorService(ExecutorService executorService, boolean shutdownOnClose)
{
- this.executorService = executorService;
+ this.executorService = Preconditions.checkNotNull(executorService);
--- End diff --
done
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-55050094
I'm not a fan of squash commits. I guess I'm pedantic and want to see all the commits. See if another committer wants to do this.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-54388198
Sorry - I just started a new job and haven’t had much time. I’ll look more soon.
-JZ
On September 3, 2014 at 7:18:54 PM, Scott Blum (notifications@github.com) wrote:
Ping. Any outstanding issues?
—
Reply to this email directly or view it on GitHub.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/41
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16621647
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +65,106 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path) {
+ this.client = client;
+ this.path = path;
--- End diff --
Great call
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16843839
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -580,13 +670,33 @@ public Void apply(TreeCacheListener listener)
}
/**
- * Default behavior is just to log the exception
- *
- * @param e the exception
+ * Send an exception to any listeners, or else log the error if there are none.
*/
- protected void handleException(Throwable e)
+ private void handleException(final Throwable e)
{
- LOG.error("", e);
+ if ( errorListeners.size() == 0 )
+ {
+ LOG.error("", e);
+ }
+ else
+ {
+ errorListeners.forEach(new Function<UnhandledErrorListener, Void>()
+ {
+ @Override
+ public Void apply(UnhandledErrorListener listener)
+ {
+ try
+ {
+ listener.unhandledError("", e);
+ }
+ catch ( Exception e )
+ {
+ LOG.error("Exception handling exception", e);
+ }
+ return null;
+ }
+ });
+ }
--- End diff --
I'd rather see a global change for something like this instead of each recipe doing it.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16621573
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +65,106 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path) {
+ this.client = client;
+ this.path = path;
--- End diff --
Please use ```PathUtils.validatePath``` to make sure that the path is valid before accepting it.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-53213724
A few minor formatting nit picks. I agree with Mike that it would be good to have some unit tests that are using the builder. Other than that, all looks good.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-58104626
Probably want a committer to weigh in, but you've addressed my concerns.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-55050374
All commits is fine. I'm pedantic and like to see JIRAs referenced in all commit messages. Fair compromise?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16851987
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -461,11 +546,16 @@ public void close()
*
* @return listenable
*/
- public ListenerContainer<TreeCacheListener> getListenable()
+ public Listenable<TreeCacheListener> getListenable()
--- End diff --
Good point. We should do this everywhere.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16843738
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -461,11 +546,16 @@ public void close()
*
* @return listenable
*/
- public ListenerContainer<TreeCacheListener> getListenable()
+ public Listenable<TreeCacheListener> getListenable()
--- End diff --
I don't understand this change. There can be multiple listeners.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16939659
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -580,13 +670,33 @@ public Void apply(TreeCacheListener listener)
}
/**
- * Default behavior is just to log the exception
- *
- * @param e the exception
+ * Send an exception to any listeners, or else log the error if there are none.
*/
- protected void handleException(Throwable e)
+ private void handleException(final Throwable e)
{
- LOG.error("", e);
+ if ( errorListeners.size() == 0 )
+ {
+ LOG.error("", e);
+ }
+ else
+ {
+ errorListeners.forEach(new Function<UnhandledErrorListener, Void>()
+ {
+ @Override
+ public Void apply(UnhandledErrorListener listener)
+ {
+ try
+ {
+ listener.unhandledError("", e);
+ }
+ catch ( Exception e )
+ {
+ LOG.error("Exception handling exception", e);
+ }
+ return null;
+ }
+ });
+ }
--- End diff --
I don't think that funneling unhandled errors through the CuratorFramework's listeners would work super well. Because there's not going to any real way of identifying where they came from, beyond logging them, I'm not sure what else you could do with them.
Maybe attaching UnhnadlerErrorListener's to each recipe is the way to go.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r17332712
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/BaseTestTreeCache.java ---
@@ -57,22 +60,38 @@ public void childEvent(CuratorFramework client, TreeCacheEvent event) throws Exc
};
/**
- * A TreeCache that records exceptions and automatically adds a listener.
+ * Ensures that tests don't cause any background errors.
*/
- class MyTreeCache extends TreeCache
+ final UnhandledErrorListener errorListener = new UnhandledErrorListener()
{
-
- MyTreeCache(CuratorFramework client, String path, boolean cacheData)
- {
- super(client, path, cacheData);
- getListenable().addListener(eventListener);
- }
-
@Override
- protected void handleException(Throwable e)
+ public void unhandledError(String message, Throwable e)
{
- handleBackgroundException(e);
+ hadBackgroundException.set(true);
+ e.printStackTrace(System.err);
}
+ };
+
+ /**
+ * Construct a TreeCache that records exceptions and automatically listens.
+ */
+ protected TreeCache newTreeCacheWithListeners(CuratorFramework client, String path)
--- End diff --
This can call buildWithListeners, since they do nearly the same thing already.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16624162
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -356,51 +456,24 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
};
- private static final ThreadFactory defaultThreadFactory = ThreadUtils.newThreadFactory("TreeCache");
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData)
- {
- this(client, path, cacheData, false, new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory), true));
- }
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param threadFactory factory to use when creating internal threads
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData, ThreadFactory threadFactory)
- {
- this(client, path, cacheData, false, new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory), true));
- }
+ static final ThreadFactory defaultThreadFactory = ThreadUtils.newThreadFactory("TreeCache");
/**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param dataIsCompressed if true, data in the path is compressed
- * @param threadFactory factory to use when creating internal threads
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData, boolean dataIsCompressed, ThreadFactory threadFactory)
- {
- this(client, path, cacheData, dataIsCompressed, new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory), true));
- }
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param dataIsCompressed if true, data in the path is compressed
- * @param executorService ExecutorService to use for the TreeCache's background thread
+ * Create a TreeCache for the given client and path with default options.
+ *
+ * If the client is namespaced, all operations on the resulting TreeCache will be in terms of
+ * the namespace, including all published events. The given path is the root at which the
+ * TreeCache will watch and explore. If no node exists at the given path, the TreeCache will
+ * be initially empty.
+ *
+ * @see #newBuilder(CuratorFramework, String)
+ * @param client the client to use; may be namespaced
+ * @param path the path to the root node to watch/explore; this path need not actually exist on
+ * the server
*/
- public TreeCache(CuratorFramework client, String path, boolean cacheData, boolean dataIsCompressed, final ExecutorService executorService)
+ public TreeCache(CuratorFramework client, String path)
--- End diff --
That makes sense. The added javadoc is probably enough to direct people towards the builder if they need something more advanced, so unless this turns out to be an issue later, then we don't need to worry about it.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16853269
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +69,112 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder
+ {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path)
+ {
+ this.client = checkNotNull(client);
+ this.path = validatePath(path);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values.
+ */
+ public TreeCache build()
+ {
+ CloseableExecutorService executor = executorService;
+ if ( executor == null )
+ {
+ executor = new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory));
+ }
+ return new TreeCache(client, path, cacheData, dataIsCompressed, executor);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values, and starts it.
+ */
+ public TreeCache buildAndStart() throws Exception
--- End diff --
Maybe start should `return this` so it's just `foo = builder.build().start()`?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-53509753
Made several fixes:
- formatting
- removed MyTreeCache in favor of using helper methods in tests that call the main constructor and builder; see what you think
- a couple of test additions
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16750582
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -461,11 +546,16 @@ public void close()
*
* @return listenable
*/
- public ListenerContainer<TreeCacheListener> getListenable()
+ public Listenable<TreeCacheListener> getListenable()
--- End diff --
this was wrong all along, a public caller shouldn't be able to e.g. fire listeners
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r17332515
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +70,102 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder
--- End diff --
Minor: can we rename this to something more specific like TreeCacheBuilder?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r18364761
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +70,102 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder
--- End diff --
That's kind of weird since it's then TreeCache.TreeCacheBuilder instead of just TreeCache.Builder. I see "Builder" as a static inner class much more often, for example in Guava (ImmutableList.Builder) and protoc-generated java code.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16638814
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +67,106 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path) {
+ this.client = checkNotNull(client);
+ this.path = validatePath(path);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values.
+ */
+ public TreeCache build()
+ {
+ CloseableExecutorService executor = executorService;
+ if ( executor == null )
+ {
+ executor = new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory));
+ }
+ return new TreeCache(client, path, cacheData, dataIsCompressed, executor);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values, and starts it.
+ */
+ public TreeCache buildAndStart() throws Exception
+ {
+ TreeCache treeCache = build();
+ treeCache.start();
+ return treeCache;
+ }
+
+ /**
+ * Sets whether or not to cache byte data per node; default {@code true}.
+ */
+ public Builder setCacheData(boolean cacheData)
+ {
+ this.cacheData = cacheData;
+ return this;
+ }
+
+ /**
+ * Sets whether or to decompress node data; default {@code false}.
+ */
+ public Builder setDataIsCompressed(boolean dataIsCompressed)
+ {
+ this.dataIsCompressed = dataIsCompressed;
+ return this;
+ }
+
+ /**
+ * Sets the executor to publish events; a default executor will be created if not specified.
+ */
+ public Builder setExecutor(ThreadFactory threadFactory)
+ {
+ return setExecutor(new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory)));
+ }
+
+ /**
+ * Sets the executor to publish events; a default executor will be created if not specified.
+ */
+ public Builder setExecutor(ExecutorService executorService)
+ {
+ if (executorService instanceof CloseableExecutorService) {
+ return setExecutor((CloseableExecutorService) executorService);
+ } else {
+ return setExecutor(new CloseableExecutorService(executorService));
+ }
+ }
+
+ /**
+ * Sets the executor to publish events; a default executor will be created if not specified.
+ */
+ public Builder setExecutor(CloseableExecutorService executorService)
+ {
+ this.executorService = checkNotNull(executorService);
+ return this;
+ }
+ }
+
+ /**
+ * Create a TreeCache builder for the given client and path to configure advanced options.
+ *
+ * If the client is namespaced, all operations on the resulting TreeCache will be in terms of
+ * the namespace, including all published events. The given path is the root at which the
+ * TreeCache will watch and explore. If no node exists at the given path, the TreeCache will
+ * be initially empty.
+ *
+ * @param client the client to use; may be namespaced
+ * @param path the path to the root node to watch/explore; this path need not actually exist on
+ * the server
+ * @return a new builder
+ */
+ public static Builder newBuilder(CuratorFramework client, String path) {
+ return new Builder(client, path);
--- End diff --
nit: formatting
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-57699318
Sounds like we're good to merge?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16621936
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -356,51 +456,24 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
};
- private static final ThreadFactory defaultThreadFactory = ThreadUtils.newThreadFactory("TreeCache");
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData)
- {
- this(client, path, cacheData, false, new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory), true));
- }
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param threadFactory factory to use when creating internal threads
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData, ThreadFactory threadFactory)
- {
- this(client, path, cacheData, false, new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory), true));
- }
+ static final ThreadFactory defaultThreadFactory = ThreadUtils.newThreadFactory("TreeCache");
/**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param dataIsCompressed if true, data in the path is compressed
- * @param threadFactory factory to use when creating internal threads
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData, boolean dataIsCompressed, ThreadFactory threadFactory)
- {
- this(client, path, cacheData, dataIsCompressed, new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory), true));
- }
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param dataIsCompressed if true, data in the path is compressed
- * @param executorService ExecutorService to use for the TreeCache's background thread
+ * Create a TreeCache for the given client and path with default options.
+ *
+ * If the client is namespaced, all operations on the resulting TreeCache will be in terms of
+ * the namespace, including all published events. The given path is the root at which the
+ * TreeCache will watch and explore. If no node exists at the given path, the TreeCache will
+ * be initially empty.
+ *
+ * @see #newBuilder(CuratorFramework, String)
+ * @param client the client to use; may be namespaced
+ * @param path the path to the root node to watch/explore; this path need not actually exist on
+ * the server
*/
- public TreeCache(CuratorFramework client, String path, boolean cacheData, boolean dataIsCompressed, final ExecutorService executorService)
+ public TreeCache(CuratorFramework client, String path)
--- End diff --
It's a convenience for the happy path where you don't need any extra options.
`new TreeCache(client, "/")` vs. `TreeCache.newBuilder(client, "/").build()`
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-55046217
Minor: before committing to master, I think it would be good to either squash everything into a single commit or edit the commit messages to all include the associated JIRA. Not sure if there is an adopted project wide policy relating to this though.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-54388042
Ping. Any outstanding issues?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16638810
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +67,106 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path) {
+ this.client = checkNotNull(client);
+ this.path = validatePath(path);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values.
+ */
+ public TreeCache build()
+ {
+ CloseableExecutorService executor = executorService;
+ if ( executor == null )
+ {
+ executor = new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory));
+ }
+ return new TreeCache(client, path, cacheData, dataIsCompressed, executor);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values, and starts it.
+ */
+ public TreeCache buildAndStart() throws Exception
+ {
+ TreeCache treeCache = build();
+ treeCache.start();
+ return treeCache;
+ }
+
+ /**
+ * Sets whether or not to cache byte data per node; default {@code true}.
+ */
+ public Builder setCacheData(boolean cacheData)
+ {
+ this.cacheData = cacheData;
+ return this;
+ }
+
+ /**
+ * Sets whether or to decompress node data; default {@code false}.
+ */
+ public Builder setDataIsCompressed(boolean dataIsCompressed)
+ {
+ this.dataIsCompressed = dataIsCompressed;
+ return this;
+ }
+
+ /**
+ * Sets the executor to publish events; a default executor will be created if not specified.
+ */
+ public Builder setExecutor(ThreadFactory threadFactory)
+ {
+ return setExecutor(new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory)));
+ }
+
+ /**
+ * Sets the executor to publish events; a default executor will be created if not specified.
+ */
+ public Builder setExecutor(ExecutorService executorService)
+ {
+ if (executorService instanceof CloseableExecutorService) {
+ return setExecutor((CloseableExecutorService) executorService);
+ } else {
+ return setExecutor(new CloseableExecutorService(executorService));
+ }
--- End diff --
nit: formatting
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-55051530
If the branch is named the same as the Jira it's not necessary to put the Jira in the commit.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16638791
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +67,106 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder {
--- End diff --
nit: Formatting
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16851772
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -461,11 +546,16 @@ public void close()
*
* @return listenable
*/
- public ListenerContainer<TreeCacheListener> getListenable()
+ public Listenable<TreeCacheListener> getListenable()
--- End diff --
It's just a surface area reduction. Returning Listenable only allows a caller to add and remove listeners (which you want). Returning the subtype ListenableContainer would let a caller manipulate the collection, i.e. size(), clear(), and forEach(). How weird would it be for an external caller to be able to do `cache.getListenable().forEach(...)`?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16750643
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -580,13 +670,33 @@ public Void apply(TreeCacheListener listener)
}
/**
- * Default behavior is just to log the exception
- *
- * @param e the exception
+ * Send an exception to any listeners, or else log the error if there are none.
*/
- protected void handleException(Throwable e)
+ private void handleException(final Throwable e)
{
- LOG.error("", e);
+ if ( errorListeners.size() == 0 )
+ {
+ LOG.error("", e);
+ }
+ else
+ {
+ errorListeners.forEach(new Function<UnhandledErrorListener, Void>()
+ {
+ @Override
+ public Void apply(UnhandledErrorListener listener)
+ {
+ try
+ {
+ listener.unhandledError("", e);
+ }
+ catch ( Exception e )
+ {
+ LOG.error("Exception handling exception", e);
+ }
+ return null;
+ }
+ });
+ }
--- End diff --
What do you guys think about using an UnhandledErrorListener this way? Seems nice to be able to handle background errors programmatically instead of always going to the logger. I needed this for testing anyway, because I'm tired of having to subclass TreeCache and override.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16960356
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -580,13 +670,33 @@ public Void apply(TreeCacheListener listener)
}
/**
- * Default behavior is just to log the exception
- *
- * @param e the exception
+ * Send an exception to any listeners, or else log the error if there are none.
*/
- protected void handleException(Throwable e)
+ private void handleException(final Throwable e)
{
- LOG.error("", e);
+ if ( errorListeners.size() == 0 )
+ {
+ LOG.error("", e);
+ }
+ else
+ {
+ errorListeners.forEach(new Function<UnhandledErrorListener, Void>()
+ {
+ @Override
+ public Void apply(UnhandledErrorListener listener)
+ {
+ try
+ {
+ listener.unhandledError("", e);
+ }
+ catch ( Exception e )
+ {
+ LOG.error("Exception handling exception", e);
+ }
+ return null;
+ }
+ });
+ }
--- End diff --
That was kind of my thought. If we're not sure, I could make this package-protected for now.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16852092
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +69,112 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder
+ {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path)
+ {
+ this.client = checkNotNull(client);
+ this.path = validatePath(path);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values.
+ */
+ public TreeCache build()
+ {
+ CloseableExecutorService executor = executorService;
+ if ( executor == null )
+ {
+ executor = new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory));
+ }
+ return new TreeCache(client, path, cacheData, dataIsCompressed, executor);
+ }
+
+ /**
+ * Builds the {@link TreeCache} based on configured values, and starts it.
+ */
+ public TreeCache buildAndStart() throws Exception
--- End diff --
This seems like an unnecessary API bloat. Users can just do build().start();
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16851906
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -580,13 +670,33 @@ public Void apply(TreeCacheListener listener)
}
/**
- * Default behavior is just to log the exception
- *
- * @param e the exception
+ * Send an exception to any listeners, or else log the error if there are none.
*/
- protected void handleException(Throwable e)
+ private void handleException(final Throwable e)
{
- LOG.error("", e);
+ if ( errorListeners.size() == 0 )
+ {
+ LOG.error("", e);
+ }
+ else
+ {
+ errorListeners.forEach(new Function<UnhandledErrorListener, Void>()
+ {
+ @Override
+ public Void apply(UnhandledErrorListener listener)
+ {
+ try
+ {
+ listener.unhandledError("", e);
+ }
+ catch ( Exception e )
+ {
+ LOG.error("Exception handling exception", e);
+ }
+ return null;
+ }
+ });
+ }
--- End diff --
Which part, having unhandledError listeners attached to a recipe? Logging to error only when there are no listeners?
Another option I considered is somehow making the recipe fire the unhandled error through the CuratorFramework's unhandledError listeners. But I wasn't sure if that was a good idea or not. WDTY?
My immediate need is to ensure during tests that no unhandled errors fire. I was doing that via subclass but I was unhappy about that approach since I want to use the Builder in tests.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-53120933
None of the unit tests use the builder, it would be good to get some coverage on it just in case.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16621651
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -356,51 +456,24 @@ public void stateChanged(CuratorFramework client, ConnectionState newState)
}
};
- private static final ThreadFactory defaultThreadFactory = ThreadUtils.newThreadFactory("TreeCache");
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData)
- {
- this(client, path, cacheData, false, new CloseableExecutorService(Executors.newSingleThreadExecutor(defaultThreadFactory), true));
- }
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param threadFactory factory to use when creating internal threads
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData, ThreadFactory threadFactory)
- {
- this(client, path, cacheData, false, new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory), true));
- }
+ static final ThreadFactory defaultThreadFactory = ThreadUtils.newThreadFactory("TreeCache");
/**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param dataIsCompressed if true, data in the path is compressed
- * @param threadFactory factory to use when creating internal threads
- */
- public TreeCache(CuratorFramework client, String path, boolean cacheData, boolean dataIsCompressed, ThreadFactory threadFactory)
- {
- this(client, path, cacheData, dataIsCompressed, new CloseableExecutorService(Executors.newSingleThreadExecutor(threadFactory), true));
- }
-
- /**
- * @param client the client
- * @param path path to watch
- * @param cacheData if true, node contents are cached in addition to the stat
- * @param dataIsCompressed if true, data in the path is compressed
- * @param executorService ExecutorService to use for the TreeCache's background thread
+ * Create a TreeCache for the given client and path with default options.
+ *
+ * If the client is namespaced, all operations on the resulting TreeCache will be in terms of
+ * the namespace, including all published events. The given path is the root at which the
+ * TreeCache will watch and explore. If no node exists at the given path, the TreeCache will
+ * be initially empty.
+ *
+ * @see #newBuilder(CuratorFramework, String)
+ * @param client the client to use; may be namespaced
+ * @param path the path to the root node to watch/explore; this path need not actually exist on
+ * the server
*/
- public TreeCache(CuratorFramework client, String path, boolean cacheData, boolean dataIsCompressed, final ExecutorService executorService)
+ public TreeCache(CuratorFramework client, String path)
--- End diff --
Is it confusing to have both a public constructor and a builder available? I think I'd prefer to expose only the builder.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-58104436
Any objections to merge?
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16843905
--- Diff: curator-client/src/main/java/org/apache/curator/utils/CloseableExecutorService.java ---
@@ -116,7 +116,7 @@ public CloseableExecutorService(ExecutorService executorService)
*/
public CloseableExecutorService(ExecutorService executorService, boolean shutdownOnClose)
{
- this.executorService = executorService;
+ this.executorService = Preconditions.checkNotNull(executorService);
--- End diff --
Add a message to the checkNotNull. It helps in debugging. The canonical form is:
```java
foo = Preconditions.checkNotNull(foo, "foo cannot be null");
```
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/41#issuecomment-58104896
Good to merge as far as I'm concerned.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r18364840
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/BaseTestTreeCache.java ---
@@ -57,22 +60,38 @@ public void childEvent(CuratorFramework client, TreeCacheEvent event) throws Exc
};
/**
- * A TreeCache that records exceptions and automatically adds a listener.
+ * Ensures that tests don't cause any background errors.
*/
- class MyTreeCache extends TreeCache
+ final UnhandledErrorListener errorListener = new UnhandledErrorListener()
{
-
- MyTreeCache(CuratorFramework client, String path, boolean cacheData)
- {
- super(client, path, cacheData);
- getListenable().addListener(eventListener);
- }
-
@Override
- protected void handleException(Throwable e)
+ public void unhandledError(String message, Throwable e)
{
- handleBackgroundException(e);
+ hadBackgroundException.set(true);
+ e.printStackTrace(System.err);
}
+ };
+
+ /**
+ * Construct a TreeCache that records exceptions and automatically listens.
+ */
+ protected TreeCache newTreeCacheWithListeners(CuratorFramework client, String path)
--- End diff --
I'm exercising two different code paths on purpose, the first the public ctor, the second the builder.
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16638793
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -65,6 +67,106 @@
{
private static final Logger LOG = LoggerFactory.getLogger(TreeCache.class);
+ public static final class Builder {
+ private final CuratorFramework client;
+ private final String path;
+ private boolean cacheData = true;
+ private boolean dataIsCompressed = false;
+ private CloseableExecutorService executorService = null;
+
+ private Builder(CuratorFramework client, String path) {
--- End diff --
nit: Formatting
---
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: CURATOR-144: TreeCache.Builder to configure ...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/41#discussion_r16966080
--- Diff: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/TreeCache.java ---
@@ -580,13 +670,33 @@ public Void apply(TreeCacheListener listener)
}
/**
- * Default behavior is just to log the exception
- *
- * @param e the exception
+ * Send an exception to any listeners, or else log the error if there are none.
*/
- protected void handleException(Throwable e)
+ private void handleException(final Throwable e)
{
- LOG.error("", e);
+ if ( errorListeners.size() == 0 )
+ {
+ LOG.error("", e);
+ }
+ else
+ {
+ errorListeners.forEach(new Function<UnhandledErrorListener, Void>()
+ {
+ @Override
+ public Void apply(UnhandledErrorListener listener)
+ {
+ try
+ {
+ listener.unhandledError("", e);
+ }
+ catch ( Exception e )
+ {
+ LOG.error("Exception handling exception", e);
+ }
+ return null;
+ }
+ });
+ }
--- End diff --
That sounds OK to me for now. I'm not sure how a global change for all recipes would look. Either a util implementation of the handleException() that you've implemented, or a base class for all recipes that holds a reference to error listeners and implements the handleException() method.
---
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.
---