You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by eolivelli <gi...@git.apache.org> on 2017/04/13 04:58:44 UTC
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
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 <eo...@apache.org>
Date: 2017-04-13T04:55:43Z
ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty and NettyServerCnxn in order to use Netty Local transport
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by ivankelly <gi...@git.apache.org>.
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by hanm <gi...@git.apache.org>.
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.).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by eolivelli <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
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)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on the issue:
https://github.com/apache/zookeeper/pull/227
@ivankelly @afine @hanm
I am going to close this pull request and the associated JIRA ticket if this approach is not interesting
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by ivankelly <gi...@git.apache.org>.
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)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by eolivelli <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli closed the pull request at:
https://github.com/apache/zookeeper/pull/227
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by eolivelli <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by eolivelli <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by ivankelly <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSocketNetty...
Posted by ivankelly <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #227: ZOOKEEPER-2755 Allow to subclass ClientCnxnSock...
Posted by afine <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---