You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2025/09/05 22:35:52 UTC
Re: [PR] ZOOKEEPER-233: Split `zookeeper` module to provide separated `zookeeper-client`, `zookeeper-server` [zookeeper]
ctubbsii commented on code in PR #2307:
URL: https://github.com/apache/zookeeper/pull/2307#discussion_r2326083135
##########
zookeeper-assembly/pom.xml:
##########
@@ -64,12 +64,6 @@
<artifactId>zookeeper</artifactId>
<version>${project.version}</version>
</dependency>
- <dependency>
- <groupId>org.apache.zookeeper</groupId>
- <artifactId>zookeeper-client</artifactId>
- <version>${project.version}</version>
- <type>pom</type>
- </dependency>
Review Comment:
The client should be included in the assembly, and should be declared explicitly. This old entry brought in the (useless) intermediate pom.xml file, but dropping the `<type>pom</type>` line should be sufficient to explicitly declare the new zookeeper-client jar as a dependency of the assembly module.
Currently, it seems like the client is being included in the assembly indirectly, via a transitive dependency through the new OSGi library named simply `zookeeper` (immediately prior dependency in this file). The same seems true of the other new jars. However, they shouldn't be brought in transitively... they should be declared explicitly, so it's easier to see what is intended to be included in the assembly.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/ClientSSLReloadTest.java:
##########
@@ -25,7 +25,6 @@
import java.util.Properties;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
-import jline.internal.Log;
Review Comment:
Good catch. Probably need a checkstyle import control rule or something to catch stuff like that in future. (as a separate patch... this PR is already big enough :smiley_cat: )
##########
pom.xml:
##########
@@ -292,6 +296,7 @@
<module>zookeeper-contrib</module>
<!-- zookeeper-it needed by fatjar contrib -->
<module>zookeeper-it</module>
+ <module>zookeeper-client-c</module>
Review Comment:
This is redundant with line 68 above, where this module has already been added to the normal build.
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/ClientSSLReloadTest.java:
##########
@@ -156,7 +155,7 @@ public void certificateReloadTest() throws Exception {
try (ZooKeeper zk = new ZooKeeper(zkServer.getSecureConnectionString(), 60000, (WatchedEvent event) -> {
switch (event.getState()) {
case SyncConnected:
- l.countDown();
+ l2.countDown();
Review Comment:
Is this a bug in the master branch right now?
##########
zookeeper-client-c/pom.xml:
##########
@@ -37,6 +37,13 @@
<build>
<plugins>
+ <plugin>
+ <artifactId>maven-deploy-plugin</artifactId>
+ <configuration>
+ <!-- this module isn't to be deployed to Maven Central -->
+ <skip>true</skip>
+ </configuration>
+ </plugin>
Review Comment:
It's okay to skip deployment to Maven Central. However, it is also possible to deploy it there, if you want to. Apache Accumulo deploys a minimal source tarball to Maven Central, for example, that users can build in their environment for their system's architecture to add an optional native library. See https://repo1.maven.org/maven2/org/apache/accumulo/accumulo-native/2.1.4/
##########
zookeeper-cli/pom.xml:
##########
@@ -0,0 +1,193 @@
+<?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>parent</artifactId>
+ <version>3.10.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>zookeeper-cli</artifactId>
+ <packaging>jar</packaging>
+ <name>Apache ZooKeeper - Cli</name>
+ <description>ZooKeeper cli</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-jute</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-common</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-client</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-server</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-server</artifactId>
+ <version>${project.version}</version>
+ <type>test-jar</type>
+ <scope>test</scope>
+ </dependency>
+
+ <dependency>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-annotations</artifactId>
+ <scope>provided</scope>
+ <optional>true</optional>
+ </dependency>
+
+ <dependency>
+ <groupId>commons-cli</groupId>
+ <artifactId>commons-cli</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-collections4</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>jline</groupId>
+ <artifactId>jline</artifactId>
+ <scope>provided</scope>
+ </dependency>
+
+ <!-- server optional components -->
+ <dependency>
+ <groupId>io.dropwizard.metrics</groupId>
+ <artifactId>metrics-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.xerial.snappy</groupId>
+ <artifactId>snappy-java</artifactId>
+ <scope>test</scope>
+ </dependency>
+
+ <!-- log facilities -->
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <scope>runtime</scope>
+ <optional>true</optional>
Review Comment:
In previous discussions, there may have been a consensus to just use the test scope for these. It has the same effect as runtime+optional for other projects that depend on this artifact (in that, it won't automatically include the logback-classic implementation transitively, which is good, so it doesn't conflict with their preferred logging runtime), but makes it more clear that it's being included in the POM because this particular module still needs *some* runtime implementation for its own tests.
```suggestion
<scope>test</scope>
```
##########
zookeeper/pom.xml:
##########
@@ -0,0 +1,185 @@
+<?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>parent</artifactId>
+ <version>3.10.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>zookeeper</artifactId>
+ <packaging>jar</packaging>
+ <name>Apache ZooKeeper - Client | Server | Cli</name>
+ <description>ZooKeeper client-server-cli</description>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-jute</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-common</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-client</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-server</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.zookeeper</groupId>
+ <artifactId>zookeeper-cli</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+
+ <!-- log -->
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <scope>runtime</scope>
+ <optional>true</optional>
+ </dependency>
+
+ <!-- admin server/ -->
+ <dependency>
+ <groupId>org.eclipse.jetty</groupId>
+ <artifactId>jetty-server</artifactId>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.eclipse.jetty</groupId>
+ <artifactId>jetty-servlet</artifactId>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.eclipse.jetty</groupId>
+ <artifactId>jetty-client</artifactId>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-databind</artifactId>
+ <scope>provided</scope>
+ </dependency>
+
+ <!-- command line -->
+ <dependency>
+ <groupId>jline</groupId>
+ <artifactId>jline</artifactId>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>commons-cli</groupId>
+ <artifactId>commons-cli</artifactId>
+ <scope>provided</scope>
+ </dependency>
+
+ <!-- metricsProvider.className = "org.apache.zookeeper.metrics.impl.DefaultMetricsProvider" -->
+ <dependency>
+ <groupId>io.dropwizard.metrics</groupId>
+ <artifactId>metrics-core</artifactId>
+ <scope>provided</scope>
+ </dependency>
+
+ <!-- zookeeper.snapshot.compression.method = "snappy" -->
+ <dependency>
+ <groupId>org.xerial.snappy</groupId>
+ <artifactId>snappy-java</artifactId>
+ <scope>provided</scope>
+ </dependency>
+ </dependencies>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>copy-dependencies</id>
+ <phase>package</phase>
+ <goals>
+ <goal>copy-dependencies</goal>
+ </goals>
+ <configuration>
+ <outputDirectory>${project.build.directory}/lib</outputDirectory>
+ <overWriteReleases>false</overWriteReleases>
+ <overWriteSnapshots>true</overWriteSnapshots>
+ <excludeTransitive>false</excludeTransitive>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+
+ <plugin>
+ <groupId>org.apache.felix</groupId>
+ <artifactId>maven-bundle-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>build bundle</id>
+ <phase>package</phase>
+ <goals>
+ <goal>bundle</goal>
+ </goals>
+ </execution>
+ </executions>
Review Comment:
This section was copied from the old `zookeeper-server` module, and is for creating the ZK OSGi bundle. However, because it exists in a new POM named simply `zookeeper`, the regular jar build (non-OSGi Maven lifecycle stuff, like maven-compiler-plugin and maven-jar-plugin) still run and create what is essentially an empty `zookeeper-<version>.jar` file. Because that file is still included in the binary tarball assembly descriptor, the resulting `-bin.tar.gz` file in the assembly module still includes a `lib/zookeeper-<version>.jar` file from this POM.
A few things should change here:
1. This module's POM should probably include a comment that explains that it only exists with the name `zookeeper` to preserve compatibility with the existing OSGi bundle name.
2. This module's POM should disable the normal jar lifecycle build by changing to `<packaging>pom</package>` (the default is `jar`, and why the regular jar lifecycle plugins get executed). See https://github.com/apache/accumulo/blob/rel/2.1.4/server/native/pom.xml#L31 for a similar scenario. That should prevent the unnecessary jar from being created, and also prevent the assembly module from including it in the tarball assembly. You may need to set the packaging on the maven-bundle-plugin config, since "jar" will no longer be inherited from the module.
3. Optional: if this pom's artifact was changed from `zookeeper` to `zookeeper-osgi`, you could omit the classifier on the bundle object. So, the resulting jar would be `zookeeper-osgi-<version>.jar` rather than `zookeeper-<version>-osgi.jar`. That rename would be a slight inconvenience perhaps, but it would make it clear that the module exists for creating the osgi bundle, and not a general placeholder for other artifacts with the common name `zookeeper`.
##########
zookeeper-assembly/src/main/assembly/bin-package.xml:
##########
@@ -63,16 +62,30 @@
<fileMode>${rw.file.permission}</fileMode>
<directoryMode>${rwx.file.permission}</directoryMode>
</fileSet>
+ <fileSet>
+ <!-- ZooKeeper client generated api document -->
+ <directory>${project.basedir}/../zookeeper-client/target/apidocs</directory>
+ <outputDirectory>docs/apidocs/zookeeper-client</outputDirectory>
+ <fileMode>${rw.file.permission}</fileMode>
+ <directoryMode>${rwx.file.permission}</directoryMode>
+ </fileSet>
Review Comment:
I never noticed before that the apidocs were being included in the ZooKeeper -bin tarball. I don't think it needs to do that. ZK already publishes the javadoc jars, which is a better way to consume the apidocs for IDEs, and for manual viewing, the published apidocs on the zookeeper.apache.org site is sufficient. I think they make the tarball too big, and the build too complicated, and they just aren't needed in the tarball. This is really just an aside comment, though. It's not related to this PR specifically.
##########
zookeeper/src/main/java/org/apache/zookeeper/osgi/Osgi.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.osgi;
+
+/**
+ * <b>DON'T USE THIS.</b>
+ *
+ * <p>`maven-bundle-plugin` does not include classes if there is no source code. I have no idea why.
+ *
Review Comment:
I'm wondering if that wasn't the maven-bundle-plugin, but the regular jar lifecycle plugins that was causing a problem. I mentioned how to address that in a different comment. I'm not sure if it will help to remove this class, though.
I wonder if it might be possible, as an alternative to creating this class, to have a `package-info.java` javadoc class instead. I'm not really very familiar with the quirks of the maven-bundle-plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org