You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tillrohrmann <gi...@git.apache.org> on 2017/01/18 15:27:25 UTC

[GitHub] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

GitHub user tillrohrmann opened a pull request:

    https://github.com/apache/flink/pull/3157

    [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFactory to create ZooKeeper utility classes

    This PR is based on #3155 and #3156.
    
    This commit adds utility classes to abstract the CuratorFramework dependency from ZooKeeper
    utility classes away. That way it is possible for modules outside of flink-runtime to use
    these utility classes without facing the problem of a relocated curator dependency.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/tillrohrmann/flink mesosRemoveDynamicBinding

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/flink/pull/3157.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 #3157
    
----
commit e68688f31ead68851a0f768a7270d8bc1b5f9ac8
Author: Till Rohrmann <tr...@apache.org>
Date:   2017-01-16T13:01:10Z

    [FLINK-5496] [mesos] Relocate Mesos Protobuf dependency to avoid version conflicts
    
    Only relocate Mesos Protobuf dependency in flink-mesos. This avoids problems with Mesos
    because Flink pulls in Protobuf 2.5.0 via Flakka.

commit dd2f56568745ade036de4d0ee333e7e7fdb47400
Author: Till Rohrmann <tr...@apache.org>
Date:   2017-01-16T13:14:18Z

    [FLINK-5495] [mesos] Provide executor to ZooKeeperMesosWorkerStore
    
    The ZooKeeperMesosWorkerStore instantiates a ZooKeeperStateHandleStore which requires an
    Executor instance. This executor is now given to the ZooKeeperMesosWorkerStore.

commit 93630bac520b8fe4a223f2b724621b211293dad2
Author: Till Rohrmann <tr...@apache.org>
Date:   2017-01-18T14:06:12Z

    [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFactory to create ZooKeeper utility classes
    
    This commit adds utility classes to abstract the CuratorFramework dependency from ZooKeeper
    utility classes away. That way it is possible for modules outside of flink-runtime to use
    these utility classes without facing the problem of a relocated curator dependency.

----


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97274290
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperUtilityFactory.java ---
    @@ -0,0 +1,120 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.flink.runtime.zookeeper;
    +
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.recipes.shared.SharedCount;
    +import org.apache.curator.framework.recipes.shared.SharedValue;
    +import org.apache.flink.configuration.Configuration;
    +import org.apache.flink.runtime.util.ZooKeeperUtils;
    +
    +import java.io.Serializable;
    +import java.util.concurrent.Executor;
    +
    +/**
    + * Creates ZooKeeper utility classes without exposing the {@link CuratorFramework} dependency. The
    + * curator framework is cached in this instance and shared among all created ZooKeeper utility
    + * instances. This requires that the utility classes DO NOT close the provided curator framework.
    + *
    + * <p>The curator framework is closed by calling the {@link #close(boolean)} method.
    + */
    +public class ZooKeeperUtilityFactory {
    +
    +	private final CuratorFramework root;
    +
    +	// Facade bound to the provided path
    +	private final CuratorFramework facade;
    +
    +	public ZooKeeperUtilityFactory(Configuration configuration, String path) throws Exception {
    +		root = ZooKeeperUtils.startCuratorFramework(configuration);
    +
    +		root.newNamespaceAwareEnsurePath(path).ensure(root.getZookeeperClient());
    +		facade = root.usingNamespace(ZooKeeperUtils.generateZookeeperPath(root.getNamespace(), path));
    +	}
    +
    +	/**
    +	 * Closes the ZooKeeperUtilityFactory. This entails closing the cached {@link CuratorFramework}
    +	 * instance. If cleanup is true, then the initial path and all its children are deleted.
    +	 *
    +	 * @param cleanup deletes the initial path and all of its children to clean up
    +	 * @throws Exception when deleting the znodes
    +	 */
    +	public void close(boolean cleanup) throws Exception {
    +		if (cleanup) {
    +			facade.delete().deletingChildrenIfNeeded().forPath("/");
    +		}
    +
    +		root.close();
    +	}
    +
    +	/**
    +	 * Creates a {@link ZooKeeperStateHandleStore} instance with the provided arguments.
    +	 *
    +	 * @param zkStateHandleStorePath specifying the path in ZooKeeper to store the state handles to
    +	 * @param stateStorageHelper storing the actual state data
    +	 * @param executor to run asynchronous callbacks of the state handle store
    +	 * @param <T> Type of the state to be stored
    +	 * @return a ZooKeeperStateHandleStore instance
    +	 * @throws Exception if ZooKeeper could not create the provided state handle store path in
    +	 *     ZooKeeper
    --- End diff --
    
    As far as I know, we don't have a hard line length limit specified for Java. Personally, I try to keep it around 100 chars per line because this is in line with the Scala style. But it really depends on the statement and totally subjective here...


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97273510
  
    --- Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/MesosApplicationMasterRunner.java ---
    @@ -394,6 +395,14 @@ protected int runPrivileged(Configuration config, Configuration dynamicPropertie
     				}
     			}
     
    +			if (mesosServices != null) {
    +				try {
    +					mesosServices.close(false);
    +				} catch (Throwable tt) {
    +					LOG.error("Error closing the ZooKeeperUtilityFactory.", tt);
    --- End diff --
    
    True, will change 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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97257309
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperUtilityFactory.java ---
    @@ -0,0 +1,120 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.flink.runtime.zookeeper;
    +
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.recipes.shared.SharedCount;
    +import org.apache.curator.framework.recipes.shared.SharedValue;
    +import org.apache.flink.configuration.Configuration;
    +import org.apache.flink.runtime.util.ZooKeeperUtils;
    +
    +import java.io.Serializable;
    +import java.util.concurrent.Executor;
    +
    +/**
    + * Creates ZooKeeper utility classes without exposing the {@link CuratorFramework} dependency. The
    + * curator framework is cached in this instance and shared among all created ZooKeeper utility
    + * instances. This requires that the utility classes DO NOT close the provided curator framework.
    + *
    + * <p>The curator framework is closed by calling the {@link #close(boolean)} method.
    + */
    +public class ZooKeeperUtilityFactory {
    +
    +	private final CuratorFramework root;
    +
    +	// Facade bound to the provided path
    +	private final CuratorFramework facade;
    +
    +	public ZooKeeperUtilityFactory(Configuration configuration, String path) throws Exception {
    --- End diff --
    
    Should perform precondition checks for arguments.


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97255788
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/util/ZooKeeperUtils.java ---
    @@ -376,15 +376,19 @@ public static ZooKeeperCheckpointIDCounter createCheckpointIDCounter(
     		}
     	}
     
    -	private static String generateZookeeperPath(String root, String namespace) {
    +	public static String generateZookeeperPath(String root, String namespace) {
     		if (!namespace.startsWith("/")) {
    -			namespace = "/" + namespace;
    +			namespace = '/' + namespace;
     		}
     
     		if (namespace.endsWith("/")) {
     			namespace = namespace.substring(0, namespace.length() - 1);
     		}
     
    +		if (root.endsWith("/")) {
    --- End diff --
    
    Nice catch ..


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97273730
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperVersionedValue.java ---
    @@ -0,0 +1,43 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.flink.runtime.zookeeper;
    +
    +import org.apache.curator.framework.recipes.shared.VersionedValue;
    +import org.apache.flink.util.Preconditions;
    +
    +/**
    + * Wrapper class for a {@link VersionedValue} so that we don't expose a curator dependency in our
    + * internal APIs. Such an exposure is problematic due to the relocation of curator.
    + */
    +public class ZooKeeperVersionedValue<T> {
    +
    +	private final VersionedValue<T> versionedValue;
    +
    +	public ZooKeeperVersionedValue(VersionedValue<T> versionedValue) {
    +		this.versionedValue = Preconditions.checkNotNull(versionedValue);
    +	}
    +
    +	VersionedValue<T> getVersionedValue() {
    --- End diff --
    
    Good point. Will change 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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97255571
  
    --- Diff: flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/MesosFlinkResourceManagerTest.java ---
    @@ -73,13 +74,16 @@
     /**
      * General tests for the Mesos resource manager component.
      */
    -public class MesosFlinkResourceManagerTest {
    +public class MesosFlinkResourceManagerTest extends TestLogger {
     
     	private static final Logger LOG = LoggerFactory.getLogger(MesosFlinkResourceManagerTest.class);
     
     	private static ActorSystem system;
     
    -	private static Configuration config = new Configuration() {{
    +	private static Configuration config = new Configuration() {
    +		private static final long serialVersionUID = -952579203067648838L;
    +
    +		{
    --- End diff --
    
    The indentation in this static block seems to be disordered (the following 2 `setInteger`s should be indented with one more tab).


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97256203
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperVersionedValue.java ---
    @@ -0,0 +1,43 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.flink.runtime.zookeeper;
    +
    +import org.apache.curator.framework.recipes.shared.VersionedValue;
    +import org.apache.flink.util.Preconditions;
    +
    +/**
    + * Wrapper class for a {@link VersionedValue} so that we don't expose a curator dependency in our
    + * internal APIs. Such an exposure is problematic due to the relocation of curator.
    + */
    +public class ZooKeeperVersionedValue<T> {
    +
    +	private final VersionedValue<T> versionedValue;
    +
    +	public ZooKeeperVersionedValue(VersionedValue<T> versionedValue) {
    +		this.versionedValue = Preconditions.checkNotNull(versionedValue);
    +	}
    +
    +	VersionedValue<T> getVersionedValue() {
    --- End diff --
    
    nitpick: I would place the public methods before any other package-private / private access methods.


---
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] flink issue #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFactory to...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:

    https://github.com/apache/flink/pull/3157
  
    Thanks for the review @tzulitai. I'll address your comments and then merge this PR.


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97257878
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperUtilityFactory.java ---
    @@ -0,0 +1,120 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.flink.runtime.zookeeper;
    +
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.recipes.shared.SharedCount;
    +import org.apache.curator.framework.recipes.shared.SharedValue;
    +import org.apache.flink.configuration.Configuration;
    +import org.apache.flink.runtime.util.ZooKeeperUtils;
    +
    +import java.io.Serializable;
    +import java.util.concurrent.Executor;
    +
    +/**
    + * Creates ZooKeeper utility classes without exposing the {@link CuratorFramework} dependency. The
    + * curator framework is cached in this instance and shared among all created ZooKeeper utility
    + * instances. This requires that the utility classes DO NOT close the provided curator framework.
    + *
    + * <p>The curator framework is closed by calling the {@link #close(boolean)} method.
    + */
    +public class ZooKeeperUtilityFactory {
    +
    +	private final CuratorFramework root;
    +
    +	// Facade bound to the provided path
    +	private final CuratorFramework facade;
    +
    +	public ZooKeeperUtilityFactory(Configuration configuration, String path) throws Exception {
    +		root = ZooKeeperUtils.startCuratorFramework(configuration);
    +
    +		root.newNamespaceAwareEnsurePath(path).ensure(root.getZookeeperClient());
    +		facade = root.usingNamespace(ZooKeeperUtils.generateZookeeperPath(root.getNamespace(), path));
    +	}
    +
    +	/**
    +	 * Closes the ZooKeeperUtilityFactory. This entails closing the cached {@link CuratorFramework}
    +	 * instance. If cleanup is true, then the initial path and all its children are deleted.
    +	 *
    +	 * @param cleanup deletes the initial path and all of its children to clean up
    +	 * @throws Exception when deleting the znodes
    +	 */
    +	public void close(boolean cleanup) throws Exception {
    +		if (cleanup) {
    +			facade.delete().deletingChildrenIfNeeded().forPath("/");
    +		}
    +
    +		root.close();
    +	}
    +
    +	/**
    +	 * Creates a {@link ZooKeeperStateHandleStore} instance with the provided arguments.
    +	 *
    +	 * @param zkStateHandleStorePath specifying the path in ZooKeeper to store the state handles to
    +	 * @param stateStorageHelper storing the actual state data
    +	 * @param executor to run asynchronous callbacks of the state handle store
    +	 * @param <T> Type of the state to be stored
    +	 * @return a ZooKeeperStateHandleStore instance
    +	 * @throws Exception if ZooKeeper could not create the provided state handle store path in
    +	 *     ZooKeeper
    --- End diff --
    
    nit: Does this line in the Javadoc need to be split into 2 lines? I don't think it's exceeding the max character limit, is 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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97273631
  
    --- Diff: flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/MesosFlinkResourceManagerTest.java ---
    @@ -73,13 +74,16 @@
     /**
      * General tests for the Mesos resource manager component.
      */
    -public class MesosFlinkResourceManagerTest {
    +public class MesosFlinkResourceManagerTest extends TestLogger {
     
     	private static final Logger LOG = LoggerFactory.getLogger(MesosFlinkResourceManagerTest.class);
     
     	private static ActorSystem system;
     
    -	private static Configuration config = new Configuration() {{
    +	private static Configuration config = new Configuration() {
    +		private static final long serialVersionUID = -952579203067648838L;
    +
    +		{
    --- End diff --
    
    Good catch. Will change 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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97273823
  
    --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/zookeeper/ZooKeeperUtilityFactory.java ---
    @@ -0,0 +1,120 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.flink.runtime.zookeeper;
    +
    +import org.apache.curator.framework.CuratorFramework;
    +import org.apache.curator.framework.recipes.shared.SharedCount;
    +import org.apache.curator.framework.recipes.shared.SharedValue;
    +import org.apache.flink.configuration.Configuration;
    +import org.apache.flink.runtime.util.ZooKeeperUtils;
    +
    +import java.io.Serializable;
    +import java.util.concurrent.Executor;
    +
    +/**
    + * Creates ZooKeeper utility classes without exposing the {@link CuratorFramework} dependency. The
    + * curator framework is cached in this instance and shared among all created ZooKeeper utility
    + * instances. This requires that the utility classes DO NOT close the provided curator framework.
    + *
    + * <p>The curator framework is closed by calling the {@link #close(boolean)} method.
    + */
    +public class ZooKeeperUtilityFactory {
    +
    +	private final CuratorFramework root;
    +
    +	// Facade bound to the provided path
    +	private final CuratorFramework facade;
    +
    +	public ZooKeeperUtilityFactory(Configuration configuration, String path) throws Exception {
    --- End diff --
    
    Yes indeed. Will change 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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/flink/pull/3157


---
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] flink pull request #3157: [FLINK-5508] [mesos] Introduce ZooKeeperUtilityFac...

Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/3157#discussion_r97253388
  
    --- Diff: flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/MesosApplicationMasterRunner.java ---
    @@ -394,6 +395,14 @@ protected int runPrivileged(Configuration config, Configuration dynamicPropertie
     				}
     			}
     
    +			if (mesosServices != null) {
    +				try {
    +					mesosServices.close(false);
    +				} catch (Throwable tt) {
    +					LOG.error("Error closing the ZooKeeperUtilityFactory.", tt);
    --- End diff --
    
    This error message is only relevant to `ZooKeeperMesosServices`, and not `StandaloneMesosServices.`
    Should we just use "Failed to clean up and close MesosServices." here, and rely on the stack trace in logs to find out the 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.
---