You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by nkalmar <gi...@git.apache.org> on 2018/10/15 21:21:54 UTC
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
GitHub user nkalmar opened a pull request:
https://github.com/apache/zookeeper/pull/670
DO_NOT_MERGE - MAVEN MIGRATION - Separation of server and client
This is to show that unlike I thought at the start of maven migration, separating java client and server code is too much of a refactor, and even introduces package change (like ConfigException needs to be removed from QuorumPeerConfig.ConfigException as both client and server related classes throws it. Moving QuorumPeerConfig would bring too much file in common.)
Also, zookeeper-common, zookeeper-client and zookeeper-server would have similar packages, sometimes tripling the package names.
Dependency hierarchy wise server would depend on client, there's no way to make the two depend only on common.
And I didn't even start looking at the tests. Most of them depend on ZooKeeper.java and other "main" classes.
My suggestion: just leave zookeeper-server, or just zookeeper.
An email has been sent to the dev list to start the discussion on this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nkalmar/zookeeper ZOOKEEPER-3029
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/670.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 #670
----
commit dae0114a829d3f33198c76bf732b35961d5c26b7
Author: Norbert Kalmar <nk...@...>
Date: 2018-10-15T13:51:52Z
ZOOKEEPER-3029 - MAVEN MIGRATION - create pom for core modules
commit 9c0e82fe37f27910daaf20248b3253e2aacef129
Author: Norbert Kalmar <nk...@...>
Date: 2018-10-15T19:34:50Z
common-build-works
commit 0ba918a53d8204bdb5bdf7d5967d3f3382ca4a70
Author: Norbert Kalmar <nk...@...>
Date: 2018-10-15T21:12:25Z
client in progress
----
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225402885
--- Diff: zookeeper-server/pom.xml ---
@@ -0,0 +1,67 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<!--
+/**
+ * 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.
+ */
+-->
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>zookeeper-server</artifactId>
+ <name>Apache ZooKeeper - Server</name>
+ <description>ZooKeeper server</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-jute</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
--- End diff --
project.version
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225420204
--- Diff: zookeeper-client/zookeeper-client-java/pom.xml ---
@@ -0,0 +1,59 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<!--
+/**
+ * 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.
+ */
+-->
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
--- End diff --
Yes, it was like this on the master pom... I will definitely fix it in a "real" PR.
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar closed the pull request at:
https://github.com/apache/zookeeper/pull/670
---
[GitHub] zookeeper issue #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of server ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/670
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2440/
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225402543
--- Diff: zookeeper-client/zookeeper-client-java/pom.xml ---
@@ -0,0 +1,59 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<!--
+/**
+ * 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.
+ */
+-->
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
--- End diff --
3.6.0?
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225402727
--- Diff: zookeeper-jute/pom.xml ---
@@ -0,0 +1,169 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<!--
+/**
+ * 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.
+ */
+-->
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>zookeeper-jute</artifactId>
+ <name>Apache ZooKeeper - Jute</name>
+ <description>ZooKeeper jute</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.yetus</groupId>
+ <artifactId>audience-annotations</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>javacc-maven-plugin</artifactId>
+ <version>2.6</version>
+ <executions>
+ <execution>
+ <phase>generate-sources</phase>
+ <id>javacc</id>
+ <goals>
+ <goal>javacc</goal>
+ </goals>
+ <configuration>
+ <sourceDirectory>${project.basedir}/src/main/java/org/apache/jute/compiler/generated/</sourceDirectory>
+ <includes>
+ <include>rcc.jj</include>
+ </includes>
+ <lookAhead>2</lookAhead>
+ <isStatic>false</isStatic>
+ <outputDirectory>${project.build.directory}/classes/</outputDirectory>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+ <!--plugin>
+ <artifactId>maven-antrun-plugin</artifactId>
--- End diff --
This one?
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225420100
--- Diff: zookeeper-common/pom.xml ---
@@ -0,0 +1,123 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<!--
+/**
+ * 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.
+ */
+-->
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>zookeeper-common</artifactId>
+ <name>Apache ZooKeeper - Common</name>
+ <description>ZooKeeper common</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-jute</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
--- End diff --
This is a strongly work in progress, and possibly it will never make it into the codebase.
I created this PR to show that it is possibly not a good idea to seperate server and client java code :(
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225419531
--- Diff: zookeeper-client/zookeeper-client-java/src/main/java/org/apache/zookeeper/client/ZKClientConfig.java ---
@@ -23,7 +23,7 @@
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.common.ZKConfig;
-import org.apache.zookeeper.server.quorum.QuorumPeerConfig.ConfigException;
+import org.apache.zookeeper.server.quorum.ConfigException;
--- End diff --
This is breaking compatibility, we cannot port this into branch3.5 and it will make transition to 3.6 slower
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by eolivelli <gi...@git.apache.org>.
Github user eolivelli commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225402462
--- Diff: zookeeper-common/pom.xml ---
@@ -0,0 +1,123 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+<!--
+/**
+ * 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.
+ */
+-->
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>zookeeper-common</artifactId>
+ <name>Apache ZooKeeper - Common</name>
+ <description>ZooKeeper common</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-jute</artifactId>
+ <version>2.6.0-SNAPSHOT</version>
--- End diff --
project.version?
---
[GitHub] zookeeper pull request #670: DO_NOT_MERGE - MAVEN MIGRATION - Separation of ...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/670#discussion_r225422359
--- Diff: zookeeper-client/zookeeper-client-java/src/main/java/org/apache/zookeeper/client/ZKClientConfig.java ---
@@ -23,7 +23,7 @@
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.zookeeper.ZooKeeper;
import org.apache.zookeeper.common.ZKConfig;
-import org.apache.zookeeper.server.quorum.QuorumPeerConfig.ConfigException;
+import org.apache.zookeeper.server.quorum.ConfigException;
--- End diff --
Yes, one of the main reasons (not just this particular instance) that it cannot be separated "nicely".
zookeeper-common and zookeeper-client-java will probably merge back to zookeeper-server.
---