Uploaded image for project: 'ZooKeeper'
  1. ZooKeeper
  2. ZOOKEEPER-2755

Allow to subclass ClientCnxnSocketNetty and NettyServerCnxn in order to use Netty Local transport

    Details

    • Type: New Feature
    • Status: In Progress
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 3.5.2
    • Fix Version/s: None
    • Component/s: java client, server
    • Labels:
      None

      Description

      ClientCnxnSocketNetty and NettyServerCnxn use explicitly InetSocketAddress class to work with network addresses.

      We can do a little refactoring to use only SocketAddress and make it possible to create subclasses of ClientCnxnSocketNetty and NettyServerCnxn which leverage built-in Netty 'local' channels.

      Such Netty local channels do not create real sockets and so allow a simple ZooKeeper server + ZooKeeper client to be run on the same JVM without binding to real TCP endpoints.

      Usecases:

      Ability to run concurrently on the same machine tests of projects which use ZooKeeper (usually in unit tests the server and the client run inside the same JVM) without dealing with random ports and in general using less network resources

      Run simplified (standalone, all processes in the same JVM) versions of applications which need a working ZooKeeper ensemble to run.

      Note:
      Embedding ZooKeeper server + client on the same JVM has many risks and in general I think we should encourage users to do so, so I in this patch I will not provide official implementations of ClientCnxnSocketNetty and NettyServerCnxn. There will be implementations only inside the test packages, in order to test that most of the features are working with custom socket factories and in particular with the 'LocalAddress' specific subclass of SocketAddress.

      Note:
      the 'Local' sockets feature will be available on Netty 4 too

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user eolivelli opened a pull request:

          https://github.com/apache/zookeeper/pull/227

          ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty and NettyServerCnxn

          Little refactoring to use only SocketAddress instead of RemoteSocketAddress and make it possible to create subclasses of ClientCnxnSocketNetty and NettyServerCnxn which leverage built-in Netty 'local' channels.

          Such Netty local channels do not create real sockets and so allow a simple standalone ZooKeeper server + ZooKeeper client to be run on the same JVM without binding to real TCP endpoints and run concurrent test processes of the same application without dealing with random ports.

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

          $ git pull https://github.com/eolivelli/zookeeper ZOOKEEPER-2755-localaddress

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

          https://github.com/apache/zookeeper/pull/227.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 #227


          commit fa306e830964b187f11aa5d08770feeaca72cd48
          Author: eolivelli <eolivelli@apache.org>
          Date: 2017-04-13T04:55:43Z

          ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty and NettyServerCnxn in order to use Netty Local transport


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user eolivelli opened a pull request: https://github.com/apache/zookeeper/pull/227 ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty and NettyServerCnxn Little refactoring to use only SocketAddress instead of RemoteSocketAddress and make it possible to create subclasses of ClientCnxnSocketNetty and NettyServerCnxn which leverage built-in Netty 'local' channels. Such Netty local channels do not create real sockets and so allow a simple standalone ZooKeeper server + ZooKeeper client to be run on the same JVM without binding to real TCP endpoints and run concurrent test processes of the same application without dealing with random ports. You can merge this pull request into a Git repository by running: $ git pull https://github.com/eolivelli/zookeeper ZOOKEEPER-2755 -localaddress Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/227.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 #227 commit fa306e830964b187f11aa5d08770feeaca72cd48 Author: eolivelli <eolivelli@apache.org> Date: 2017-04-13T04:55:43Z ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty and NettyServerCnxn in order to use Netty Local transport
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          -1 @author. The patch appears to contain 1 @author tags which the Zookeeper community has agreed to not allow in code contributions.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/541//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/541//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/541//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build -1 @author. The patch appears to contain 1 @author tags which the Zookeeper community has agreed to not allow in code contributions. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/541//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/541//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/541//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/542//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/542//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/542//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/542//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/542//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/542//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113310620

          — Diff: src/java/main/org/apache/zookeeper/common/SocketAddressUtils.java —
          @@ -0,0 +1,97 @@
          +/* 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.zookeeper.common;
          +
          +import java.net.InetAddress;
          +import java.net.InetSocketAddress;
          +import java.net.SocketAddress;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.data.Id;
          — End diff –

          unused imports

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113310620 — Diff: src/java/main/org/apache/zookeeper/common/SocketAddressUtils.java — @@ -0,0 +1,97 @@ +/* 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.zookeeper.common; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.Id; — End diff – unused imports
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113306599

          — Diff: src/java/main/org/apache/zookeeper/server/ConnectionBean.java —
          @@ -21,15 +21,18 @@
          import java.net.Inet6Address;
          import java.net.InetAddress;
          import java.net.InetSocketAddress;
          +import java.net.SocketAddress;
          import java.util.Arrays;

          import javax.management.ObjectName;
          +import org.apache.zookeeper.common.SocketAddressUtils;

          import org.apache.zookeeper.common.Time;
          import org.slf4j.Logger;
          import org.slf4j.LoggerFactory;
          import org.apache.zookeeper.jmx.MBeanRegistry;
          import org.apache.zookeeper.jmx.ZKMBeanInfo;
          +import org.jboss.netty.channel.local.LocalAddress;
          — End diff –

          unused import

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113306599 — Diff: src/java/main/org/apache/zookeeper/server/ConnectionBean.java — @@ -21,15 +21,18 @@ import java.net.Inet6Address; import java.net.InetAddress; import java.net.InetSocketAddress; +import java.net.SocketAddress; import java.util.Arrays; import javax.management.ObjectName; +import org.apache.zookeeper.common.SocketAddressUtils; import org.apache.zookeeper.common.Time; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.apache.zookeeper.jmx.MBeanRegistry; import org.apache.zookeeper.jmx.ZKMBeanInfo; +import org.jboss.netty.channel.local.LocalAddress; — End diff – unused import
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113311911

          — Diff: src/java/main/org/apache/zookeeper/common/SocketAddressUtils.java —
          @@ -0,0 +1,97 @@
          +/* 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.zookeeper.common;
          +
          +import java.net.InetAddress;
          +import java.net.InetSocketAddress;
          +import java.net.SocketAddress;
          +import org.apache.zookeeper.KeeperException;
          +import org.apache.zookeeper.data.Id;
          +import org.jboss.netty.channel.local.LocalAddress;
          +
          +public class SocketAddressUtils {
          +
          + public static InetAddress getInetAddress(SocketAddress socketAddress) {
          + if (socketAddress instanceof InetSocketAddress)

          { + return ((InetSocketAddress) socketAddress).getAddress(); + }

          else if (socketAddress instanceof LocalAddress)

          { + return InetAddress.getLoopbackAddress(); + }

          else

          { + throw new IllegalArgumentException("Unexpected address type " + socketAddress.getClass().getName() + ": " + socketAddress.toString()); + }

          + }
          +
          + public static LocalAddress mapToLocalAddress(InetSocketAddress socketAddress) {
          + if (socketAddress.getAddress().getHostAddress().equals("0.0.0.0"))

          { + return new LocalAddress(InetAddress.getLoopbackAddress().getHostAddress() + ":" + socketAddress.getPort()); + }

          else

          { + return new LocalAddress(socketAddress.getAddress().getHostAddress() + ":" + socketAddress.getPort()); + }

          + }
          +
          + public static int getPort(SocketAddress socketAddress) {
          + if (socketAddress instanceof InetSocketAddress)

          { + return ((InetSocketAddress) socketAddress).getPort(); + }

          else if (socketAddress instanceof LocalAddress) {
          + LocalAddress local = (LocalAddress) socketAddress;
          + String id = local.getId();
          + try

          { + int colon = id.lastIndexOf(':'); + return Integer.parseInt(id.substring(colon + 1)); + }

          catch (NumberFormatException | IndexOutOfBoundsException err)

          { + throw new IllegalArgumentException("Unexpected local address " + id); + }
          + } else { + throw new IllegalArgumentException("Unexpected address type " + socketAddress.getClass().getName() + ": " + socketAddress.toString()); + }
          + }
          +
          + public static String getHostString(SocketAddress socketAddress) {
          + if (socketAddress instanceof InetSocketAddress) { + return ((InetSocketAddress) socketAddress).getHostString(); + } else if (socketAddress instanceof LocalAddress) {
          + LocalAddress local = (LocalAddress) socketAddress;
          + String id = local.getId();
          + try { + int colon = id.lastIndexOf(':'); + return id.substring(0, colon); + } catch (IndexOutOfBoundsException err) { + throw new IllegalArgumentException("Unexpected local address " + id); + }

          + } else

          { + throw new IllegalArgumentException("Unexpected address type " + socketAddress.getClass().getName() + ": " + socketAddress.toString()); + }

          + }
          +
          + public static String getHostAddress(SocketAddress socketAddress) {
          +
          + if (socketAddress instanceof InetSocketAddress)

          { + return ((InetSocketAddress) socketAddress).getAddress().getHostAddress(); + }

          else if (socketAddress instanceof LocalAddress) {
          + LocalAddress local = (LocalAddress) socketAddress;
          — End diff –

          this code seems to be shared with getHostString, is there a way that duplication can be reduced?

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113311911 — Diff: src/java/main/org/apache/zookeeper/common/SocketAddressUtils.java — @@ -0,0 +1,97 @@ +/* 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.zookeeper.common; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.Id; +import org.jboss.netty.channel.local.LocalAddress; + +public class SocketAddressUtils { + + public static InetAddress getInetAddress(SocketAddress socketAddress) { + if (socketAddress instanceof InetSocketAddress) { + return ((InetSocketAddress) socketAddress).getAddress(); + } else if (socketAddress instanceof LocalAddress) { + return InetAddress.getLoopbackAddress(); + } else { + throw new IllegalArgumentException("Unexpected address type " + socketAddress.getClass().getName() + ": " + socketAddress.toString()); + } + } + + public static LocalAddress mapToLocalAddress(InetSocketAddress socketAddress) { + if (socketAddress.getAddress().getHostAddress().equals("0.0.0.0")) { + return new LocalAddress(InetAddress.getLoopbackAddress().getHostAddress() + ":" + socketAddress.getPort()); + } else { + return new LocalAddress(socketAddress.getAddress().getHostAddress() + ":" + socketAddress.getPort()); + } + } + + public static int getPort(SocketAddress socketAddress) { + if (socketAddress instanceof InetSocketAddress) { + return ((InetSocketAddress) socketAddress).getPort(); + } else if (socketAddress instanceof LocalAddress) { + LocalAddress local = (LocalAddress) socketAddress; + String id = local.getId(); + try { + int colon = id.lastIndexOf(':'); + return Integer.parseInt(id.substring(colon + 1)); + } catch (NumberFormatException | IndexOutOfBoundsException err) { + throw new IllegalArgumentException("Unexpected local address " + id); + } + } else { + throw new IllegalArgumentException("Unexpected address type " + socketAddress.getClass().getName() + ": " + socketAddress.toString()); + } + } + + public static String getHostString(SocketAddress socketAddress) { + if (socketAddress instanceof InetSocketAddress) { + return ((InetSocketAddress) socketAddress).getHostString(); + } else if (socketAddress instanceof LocalAddress) { + LocalAddress local = (LocalAddress) socketAddress; + String id = local.getId(); + try { + int colon = id.lastIndexOf(':'); + return id.substring(0, colon); + } catch (IndexOutOfBoundsException err) { + throw new IllegalArgumentException("Unexpected local address " + id); + } + } else { + throw new IllegalArgumentException("Unexpected address type " + socketAddress.getClass().getName() + ": " + socketAddress.toString()); + } + } + + public static String getHostAddress(SocketAddress socketAddress) { + + if (socketAddress instanceof InetSocketAddress) { + return ((InetSocketAddress) socketAddress).getAddress().getHostAddress(); + } else if (socketAddress instanceof LocalAddress) { + LocalAddress local = (LocalAddress) socketAddress; — End diff – this code seems to be shared with getHostString, is there a way that duplication can be reduced?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113312598

          — Diff: src/java/main/org/apache/zookeeper/server/ConnectionBean.java —
          @@ -69,12 +72,17 @@ public String getSessionId() {
          }

          public String getSourceIP() {

          • InetSocketAddress sockAddr = connection.getRemoteSocketAddress();
            + SocketAddress sockAddr = connection.getRemoteSocketAddress();
            if (sockAddr == null) {
              • End diff –

          i believe instanceof can be safely used with null, so you can just remove this if statement

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113312598 — Diff: src/java/main/org/apache/zookeeper/server/ConnectionBean.java — @@ -69,12 +72,17 @@ public String getSessionId() { } public String getSourceIP() { InetSocketAddress sockAddr = connection.getRemoteSocketAddress(); + SocketAddress sockAddr = connection.getRemoteSocketAddress(); if (sockAddr == null) { End diff – i believe instanceof can be safely used with null, so you can just remove this if statement
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113326184

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          it seems like it can get tedious to track which tests are running with local channels. Can we turn this functionality on and off across the entire suite with a command line arg when starting the tests?

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113326184 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – it seems like it can get tedious to track which tests are running with local channels. Can we turn this functionality on and off across the entire suite with a command line arg when starting the tests?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113325191

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalChannelSuiteBase.java —
          @@ -0,0 +1,136 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import java.io.IOException;
          +import java.net.InetSocketAddress;
          +
          +import org.apache.zookeeper.ClientCnxnSocketNetty;
          +import org.apache.zookeeper.client.ZKClientConfig;
          +import org.apache.zookeeper.common.SocketAddressUtils;
          +import org.apache.zookeeper.server.NettyServerCnxnFactory;
          +import org.apache.zookeeper.server.ServerCnxnFactory;
          +import org.jboss.netty.bootstrap.ClientBootstrap;
          +import org.jboss.netty.bootstrap.ServerBootstrap;
          +import org.jboss.netty.channel.Channel;
          +import org.jboss.netty.channel.ChannelFactory;
          +import org.jboss.netty.channel.ChannelFuture;
          +import org.jboss.netty.channel.ServerChannelFactory;
          — End diff –

          unused import

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113325191 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalChannelSuiteBase.java — @@ -0,0 +1,136 @@ +/** + * 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.zookeeper.test; + +import java.io.IOException; +import java.net.InetSocketAddress; + +import org.apache.zookeeper.ClientCnxnSocketNetty; +import org.apache.zookeeper.client.ZKClientConfig; +import org.apache.zookeeper.common.SocketAddressUtils; +import org.apache.zookeeper.server.NettyServerCnxnFactory; +import org.apache.zookeeper.server.ServerCnxnFactory; +import org.jboss.netty.bootstrap.ClientBootstrap; +import org.jboss.netty.bootstrap.ServerBootstrap; +import org.jboss.netty.channel.Channel; +import org.jboss.netty.channel.ChannelFactory; +import org.jboss.netty.channel.ChannelFuture; +import org.jboss.netty.channel.ServerChannelFactory; — End diff – unused import
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113414288

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          @afine Thank you for your review.
          I have addressed most of your comments (fix imports, duplicate code...)

          Regarding the tests,
          It will be interesting in order to see that all the code can run cleanly without network.
          But IMHO a new system property/run time flag will add the need to run always twice the full suite at every build/release.
          I have added the least possible testcases to cover basic features so that future refactor will not drop the ability to use the Local Transport.
          At this moment I think it is not the time to add an official "local transport" netty connection factory.

          Regarding server-to-server communications:
          I can't find how to enable Netty for server-to-server communications, I will dig deeply into the code. It seems that the Learner class uses directly a Socket, so it is not possible to leverage Netty local transport feature.

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113414288 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – @afine Thank you for your review. I have addressed most of your comments (fix imports, duplicate code...) Regarding the tests, It will be interesting in order to see that all the code can run cleanly without network. But IMHO a new system property/run time flag will add the need to run always twice the full suite at every build/release. I have added the least possible testcases to cover basic features so that future refactor will not drop the ability to use the Local Transport. At this moment I think it is not the time to add an official "local transport" netty connection factory. Regarding server-to-server communications: I can't find how to enable Netty for server-to-server communications, I will dig deeply into the code. It seems that the Learner class uses directly a Socket, so it is not possible to leverage Netty local transport feature.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/636//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/636//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/636//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/636//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/636//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/636//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/638//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/638//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/638//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/638//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/638//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/638//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/640//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/640//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/640//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/640//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/640//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/640//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113590962

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          > But IMHO a new system property/run time flag will add the need to run always twice the full suite at every build/release.

          Maybe. We have a good degree of flakyness in our testing and wondering if using this type of change for our precommit hook will increase performance and stability (especially if we can use something similar for server<->server).

          > At this moment I think it is not the time to add an official "local transport" netty connection factory.

          Agreed.

          > I can't find how to enable Netty for server-to-server communications

          It is currently not possible. All communications there are handled by old fashioned sockets. I was incorrect in my other comment, although I still think my point is valid. Is there a way to have similar functionality with old fashioned java sockets? I don't think this change makes too much sense for testing unless we can take zk off the OS network stack entirely.

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113590962 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – > But IMHO a new system property/run time flag will add the need to run always twice the full suite at every build/release. Maybe. We have a good degree of flakyness in our testing and wondering if using this type of change for our precommit hook will increase performance and stability (especially if we can use something similar for server<->server). > At this moment I think it is not the time to add an official "local transport" netty connection factory. Agreed. > I can't find how to enable Netty for server-to-server communications It is currently not possible. All communications there are handled by old fashioned sockets. I was incorrect in my other comment, although I still think my point is valid. Is there a way to have similar functionality with old fashioned java sockets? I don't think this change makes too much sense for testing unless we can take zk off the OS network stack entirely.
          Hide
          hadoopqa Hadoop QA added a comment -

          +1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/642//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/642//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/642//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/642//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/642//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/642//console This message is automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to cause Findbugs (version 3.0.1) to fail.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/649//testReport/
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/649//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to cause Findbugs (version 3.0.1) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/649//testReport/ Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/649//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r113903695

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          @afine I'm sorry it is not possible for old style sockets. We should provide a full custom SocketImpl.

          This change will help testing apps which use ZooKeeper and usually launch only a single node embedded ZK server inside the JVM which is running the unit test.

          I think that a switch to Netty for the server-to-server communications will be a good enhancement for the future. I saw recent work about SSL on server-to-server, using Netty it would have been very simple.

          I can create a JIRA for Netty on server-to-server, eventually I can work on a proposal in the near future

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r113903695 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – @afine I'm sorry it is not possible for old style sockets. We should provide a full custom SocketImpl. This change will help testing apps which use ZooKeeper and usually launch only a single node embedded ZK server inside the JVM which is running the unit test. I think that a switch to Netty for the server-to-server communications will be a good enhancement for the future. I saw recent work about SSL on server-to-server, using Netty it would have been very simple. I can create a JIRA for Netty on server-to-server, eventually I can work on a proposal in the near future
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user afine commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r114013725

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          That makes sense. +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user afine commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r114013725 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – That makes sense. +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r114798834

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          @afine Thank you for your review

          is this patch ready for merge ?
          can you push it to the 3.5 branch too ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r114798834 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – @afine Thank you for your review is this patch ready for merge ? can you push it to the 3.5 branch too ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r115932842

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          tagging @arshadmohammad @hanm for review/merge

          @Randgalt do you think this new feature would be useful for Curator and for local testing of apps which use ZooKeeper ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r115932842 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – tagging @arshadmohammad @hanm for review/merge @Randgalt do you think this new feature would be useful for Curator and for local testing of apps which use ZooKeeper ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on a diff in the pull request:

          https://github.com/apache/zookeeper/pull/227#discussion_r120951358

          — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java —
          @@ -0,0 +1,35 @@
          +/**
          + * 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.zookeeper.test;
          +
          +import org.junit.runners.Suite;
          +
          +/**
          + * Run tests with: Netty Client against Netty server
          + */
          +@Suite.SuiteClasses({
          — End diff –

          Ping

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/227#discussion_r120951358 — Diff: src/java/test/org/apache/zookeeper/test/NettyLocalSuiteTest.java — @@ -0,0 +1,35 @@ +/** + * 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.zookeeper.test; + +import org.junit.runners.Suite; + +/** + * Run tests with: Netty Client against Netty server + */ +@Suite.SuiteClasses({ — End diff – Ping
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @ivankelly thanks for taking a look.
          I do not have numbers about local transport vs loopback.
          The first use case is to run unit tests of applications which use zk eithout opening ports. Using an ephemeral port may work but it makes trickier setting up things.
          Without opening ports you can run multiple parallel tests easily

          The second is to run applications which are made to run both in 'distributed' mode and in single process mode and use zk and other libs, especially bookkeeper without opening ports.

          Opening a port can be seen as a security risk.
          For instance I have several apps which use bk as internal wal but I need to make zk port open at least on loopback interface.

          I see that these points can be managed using SO level configs/tricks or using docker...but I if cut off the network stack if zk and bk at all it will make packaging of these apps really easy.

          I will update the commit message as well

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/227 @ivankelly thanks for taking a look. I do not have numbers about local transport vs loopback. The first use case is to run unit tests of applications which use zk eithout opening ports. Using an ephemeral port may work but it makes trickier setting up things. Without opening ports you can run multiple parallel tests easily The second is to run applications which are made to run both in 'distributed' mode and in single process mode and use zk and other libs, especially bookkeeper without opening ports. Opening a port can be seen as a security risk. For instance I have several apps which use bk as internal wal but I need to make zk port open at least on loopback interface. I see that these points can be managed using SO level configs/tricks or using docker...but I if cut off the network stack if zk and bk at all it will make packaging of these apps really easy. I will update the commit message as well
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          For my use cases server to server is not required and netty is not yet supported on that side.
          Eventually I can work in the future to introduce netty

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/227 For my use cases server to server is not required and netty is not yet supported on that side. Eventually I can work in the future to introduce netty
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ivankelly commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          Is this for tests or for production use?

          > Using an ephemeral port may work but it makes trickier setting up things.
          How does it make things tricker? Each server you set up in the test setup just needs to expose a method to query which port it is listening. This is simplier than using a whole other transport, and ensures the same code paths will be used in the test as will be used in production.

          > Without opening ports you can run multiple parallel tests easily
          Opening ports doesn't have to be hard.

          > Opening a port can be seen as a security risk.
          Tests should not be run anywhere security is a concern.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ivankelly commented on the issue: https://github.com/apache/zookeeper/pull/227 Is this for tests or for production use? > Using an ephemeral port may work but it makes trickier setting up things. How does it make things tricker? Each server you set up in the test setup just needs to expose a method to query which port it is listening. This is simplier than using a whole other transport, and ensures the same code paths will be used in the test as will be used in production. > Without opening ports you can run multiple parallel tests easily Opening ports doesn't have to be hard. > Opening a port can be seen as a security risk. Tests should not be run anywhere security is a concern.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @ivankelly I have some cases in which I will use in production. I have some app which can be installed both in clusters and in single machine deployment.
          I know zk server is not born to run inside the same process, it is tricky.
          But actually there is no other way to run bookkeeper. In the short term I am going to work in bk to abstract from zk

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/227 @ivankelly I have some cases in which I will use in production. I have some app which can be installed both in clusters and in single machine deployment. I know zk server is not born to run inside the same process, it is tricky. But actually there is no other way to run bookkeeper. In the short term I am going to work in bk to abstract from zk
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hanm commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          >> I'm not sure what the ZK convention is these days, but it would be good to have more details in the commit message about the rationale for this change, and what has changed.

          +1. I updated How To Contribute cwiki page regarding the desire of a descriptive wiki page.

          If you look at recent commits, they are actually better than the legacy commits which only contains a single line description (usually the JIRA title). Though, that might be the convention that I am not fully aware of (keep commit message concise, for those who want find more, check out the JIRA.).

          Show
          githubbot ASF GitHub Bot added a comment - Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/227 >> I'm not sure what the ZK convention is these days, but it would be good to have more details in the commit message about the rationale for this change, and what has changed. +1. I updated How To Contribute cwiki page regarding the desire of a descriptive wiki page. If you look at recent commits, they are actually better than the legacy commits which only contains a single line description (usually the JIRA title). Though, that might be the convention that I am not fully aware of (keep commit message concise, for those who want find more, check out the JIRA.).
          Hide
          rakeshr Rakesh R added a comment -

          Embedding ZooKeeper server + client on the same JVM has many risks

          Enrico Olivelli Could you elaborate on the potential risks that you anticipate.

          Show
          rakeshr Rakesh R added a comment - Embedding ZooKeeper server + client on the same JVM has many risks Enrico Olivelli Could you elaborate on the potential risks that you anticipate.
          Hide
          eolivelli Enrico Olivelli added a comment -

          Rakesh R thank you for taking a look.

          I have been running ZooKeeper server in production together with other applications which use ZK since quite some time (years).
          Usually these application have been designed to run in a distributed architecture but they are installed in standalone mode for customers which need a simple solution, do not need to process heavy loads and it is better to have only a single process/application to deploy and maintain.

          When running ZK server in the same JVM of the client application you have to ensure that ZK server has the requested amount of memory to work, no OutOfMemoryErrors, no frequent full GCs.

          You have to be sure that the ZK server boots cleanly as first component of the application and take into account that the boot will not be immediate. Actually there is no official API to boot a ZK server (for instance Curator TestingServer is prefixed 'Testing' and it is not made for production)

          An interesting issue that I have found is about ephemeral nodes and expired sessions:
          if you start/stop the ZK server together with the application, sessions opened from a "previous life" of the application will not expire during the "stop" time as no leader is running and so at the next boot ephemeral nodes created by the application are likely to be still present for a "little" (but unpredictable) time.

          Show
          eolivelli Enrico Olivelli added a comment - Rakesh R thank you for taking a look. I have been running ZooKeeper server in production together with other applications which use ZK since quite some time (years). Usually these application have been designed to run in a distributed architecture but they are installed in standalone mode for customers which need a simple solution, do not need to process heavy loads and it is better to have only a single process/application to deploy and maintain. When running ZK server in the same JVM of the client application you have to ensure that ZK server has the requested amount of memory to work, no OutOfMemoryErrors, no frequent full GCs. You have to be sure that the ZK server boots cleanly as first component of the application and take into account that the boot will not be immediate. Actually there is no official API to boot a ZK server (for instance Curator TestingServer is prefixed 'Testing' and it is not made for production) An interesting issue that I have found is about ephemeral nodes and expired sessions: if you start/stop the ZK server together with the application, sessions opened from a "previous life" of the application will not expire during the "stop" time as no leader is running and so at the next boot ephemeral nodes created by the application are likely to be still present for a "little" (but unpredictable) time.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @ivankelly I have rebased the patch to latest master and updated the commit message

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/227 @ivankelly I have rebased the patch to latest master and updated the commit message
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. GitHub Pull Request Build

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/873//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/873//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/873//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. GitHub Pull Request Build +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/873//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/873//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/873//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ivankelly commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @eolivelli
          Sorry for taking so long to respond, I was out of town.

          The commit message explains what the patch is doing, but not why. The reason I'm pushing back a lot on this, is that I think it adds indirection and complexity, and I don't see what the benefit is over simply binding to 0.

          The patch claims to abstract away the communication channel between server and client. But this is a broken abstraction as there are many places where the application expects this to be an IPv4 channel. To get around this you've created a static helper class, but internally this either forces a cast to InetSocketAddress or provides a stub, which seems hacky.

          How are you using zookeeper in single node mode? Is it only as a metadata store for bk? I'm reluctant to approve a patch that adds significant complexity without addressing a widely desirable usecase.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ivankelly commented on the issue: https://github.com/apache/zookeeper/pull/227 @eolivelli Sorry for taking so long to respond, I was out of town. The commit message explains what the patch is doing, but not why. The reason I'm pushing back a lot on this, is that I think it adds indirection and complexity, and I don't see what the benefit is over simply binding to 0. The patch claims to abstract away the communication channel between server and client. But this is a broken abstraction as there are many places where the application expects this to be an IPv4 channel. To get around this you've created a static helper class, but internally this either forces a cast to InetSocketAddress or provides a stub, which seems hacky. How are you using zookeeper in single node mode? Is it only as a metadata store for bk? I'm reluctant to approve a patch that adds significant complexity without addressing a widely desirable usecase.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @ivankelly thank you for your time

          > The commit message explains what the patch is doing, but not why. The reason I'm pushing back a lot on this, is that I think it adds indirection and complexity, and I don't see what the benefit is over simply binding to 0.

          I will rewrite the message and explain better.

          Benefits for tests: I agree with you that binding to 0 will solve the problem of running multiple tests on the same machine, this change will only add the ability to work without opening real ports.

          For production: With in-vm transport you will not open ZK clientPort to the world, which in turn will be a security risk. If you open the clientPort, even only on loopback you need to configure ZK security at ZK level or for instance iptables

          > you've created a static helper class
          Yes, unfortunately there is no automatic way to map local addresses to InetAddress. I have included the "mapToLocalAddress" method which is used only in tests in order to define a standard practice to map LocalAddress. This patch does not introduce an official CnxnFactory for Local transport, but there is a need to define how it should be used. Maybe it would be better to add the official CnxnFactory

          > How are you using zookeeper in single node mode? Is it only as a metadata store for bk?
          Yes, I am using it to run BookKeeper + Bookie inside the same JVM. I am using primary BK as write-ahead-log for replicated states machines.
          For every application now I need to implement a BookKeeper based WAL + local disk WAL, when BookKeeper is really good even in local mode.
          I really would like to abstract the metadata-store and bookie discovery in BK to not use ZK but I think this will be the work in the next year, actually (4.5 release) we are focusing the efforts on other aspects.
          I have implemented Local Transport in BK too. On BookKeeper I had the same security problem because BK 4.4 did not have "public" security support at all (in 4.5 we have it with SSL + SASL + ZK ACLs)

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/227 @ivankelly thank you for your time > The commit message explains what the patch is doing, but not why. The reason I'm pushing back a lot on this, is that I think it adds indirection and complexity, and I don't see what the benefit is over simply binding to 0. I will rewrite the message and explain better. Benefits for tests: I agree with you that binding to 0 will solve the problem of running multiple tests on the same machine, this change will only add the ability to work without opening real ports. For production: With in-vm transport you will not open ZK clientPort to the world, which in turn will be a security risk. If you open the clientPort, even only on loopback you need to configure ZK security at ZK level or for instance iptables > you've created a static helper class Yes, unfortunately there is no automatic way to map local addresses to InetAddress. I have included the "mapToLocalAddress" method which is used only in tests in order to define a standard practice to map LocalAddress. This patch does not introduce an official CnxnFactory for Local transport, but there is a need to define how it should be used. Maybe it would be better to add the official CnxnFactory > How are you using zookeeper in single node mode? Is it only as a metadata store for bk? Yes, I am using it to run BookKeeper + Bookie inside the same JVM. I am using primary BK as write-ahead-log for replicated states machines. For every application now I need to implement a BookKeeper based WAL + local disk WAL, when BookKeeper is really good even in local mode. I really would like to abstract the metadata-store and bookie discovery in BK to not use ZK but I think this will be the work in the next year, actually (4.5 release) we are focusing the efforts on other aspects. I have implemented Local Transport in BK too. On BookKeeper I had the same security problem because BK 4.4 did not have "public" security support at all (in 4.5 we have it with SSL + SASL + ZK ACLs)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ivankelly commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @eolivelli

          Only the testing usecase appears to hold value for the zookeeper community at large. Your production usecase, is really a production usecase of bookkeeper, and zookeeper is only getting caught up in it because currently bookkeeper has a hard dependency on zookeeper.

          As i understand it, you're using bookkeeper locally as a WAL logging library. Even this is not a officially supported usecase for bookkeeper, though it is something the bookkeeper community could explore. In theory it should be straight forward to create something like a local ledger interface that sends all writes to the local disk. But this is the level at which this would hold value for for the larger community.

          As such, unfortunately I'm -1 on this patch going in. I'm happy to be overriden by someone with authority on the zk repo though (cc: @hanm)

          Show
          githubbot ASF GitHub Bot added a comment - Github user ivankelly commented on the issue: https://github.com/apache/zookeeper/pull/227 @eolivelli Only the testing usecase appears to hold value for the zookeeper community at large. Your production usecase, is really a production usecase of bookkeeper, and zookeeper is only getting caught up in it because currently bookkeeper has a hard dependency on zookeeper. As i understand it, you're using bookkeeper locally as a WAL logging library. Even this is not a officially supported usecase for bookkeeper, though it is something the bookkeeper community could explore. In theory it should be straight forward to create something like a local ledger interface that sends all writes to the local disk. But this is the level at which this would hold value for for the larger community. As such, unfortunately I'm -1 on this patch going in. I'm happy to be overriden by someone with authority on the zk repo though (cc: @hanm)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user eolivelli commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          @ivankelly
          I understand your point

          This is another usecase of an use which hacked zookeeper in order to achieve a similar goal.
          http://mail-archives.apache.org/mod_mbox/zookeeper-dev/201609.mbox/%3CCACqFvNgXo%3DJQ5jrWQy3oE_ZO0%2B_9PCRtfqA0ScRk4GFd2J08pA%40mail.gmail.com%3E

          I will try to fnd another way. The only pity is that if I will implement my own connection factories I will need to duplicate all the code, you know.

          I will think about it more and will be back with a better proposal. Anyway on bookkeeper I will work on the abstraction needed not to rely on zookeeper

          Show
          githubbot ASF GitHub Bot added a comment - Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/227 @ivankelly I understand your point This is another usecase of an use which hacked zookeeper in order to achieve a similar goal. http://mail-archives.apache.org/mod_mbox/zookeeper-dev/201609.mbox/%3CCACqFvNgXo%3DJQ5jrWQy3oE_ZO0%2B_9PCRtfqA0ScRk4GFd2J08pA%40mail.gmail.com%3E I will try to fnd another way. The only pity is that if I will implement my own connection factories I will need to duplicate all the code, you know. I will think about it more and will be back with a better proposal. Anyway on bookkeeper I will work on the abstraction needed not to rely on zookeeper
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ivankelly commented on the issue:

          https://github.com/apache/zookeeper/pull/227

          >
          > I will try to fnd another way. The only pity is that if I will implement
          > my own connection factories I will need to duplicate all the code, you know.
          >
          Why would you need to implement your own connection factories? From my
          understanding of your usecase, you don't need connection factories/netty at
          all, but rather some interface which encapsulates the Journal and
          LedgerStorage in bookkeeper. Basically, something that separates out the
          entire storage layer of a bookie from the RPC layer and any other services
          that are running (like stuff for registering with zk, etc).

          Show
          githubbot ASF GitHub Bot added a comment - Github user ivankelly commented on the issue: https://github.com/apache/zookeeper/pull/227 > > I will try to fnd another way. The only pity is that if I will implement > my own connection factories I will need to duplicate all the code, you know. > Why would you need to implement your own connection factories? From my understanding of your usecase, you don't need connection factories/netty at all, but rather some interface which encapsulates the Journal and LedgerStorage in bookkeeper. Basically, something that separates out the entire storage layer of a bookie from the RPC layer and any other services that are running (like stuff for registering with zk, etc).

            People

            • Assignee:
              eolivelli Enrico Olivelli
              Reporter:
              eolivelli Enrico Olivelli
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development