ZOOKEEPER-233: Split `zookeeper` module to provide separated `zookeeper-client`, `zookeeper-server` #2307

kezhuw · 2025-08-20T17:05:57Z

Split zookeeper to separated modules:

zookeeper-common contains common codes for client and server and depends only on zookeeper-jute.
zookeeper-client contains only client side codes and depends only on zookeeper-jute and zookeeper-common. Currently, most of its tests reside in zookeeper-server still.
zookeeper-server contains only server side code and depends only on zookeeper-jute and zookeeper-common. It depend on zookeeper-client only for tests.
zookeeper-cli contains codes for command line client. It depends mainly on zookeeper-client, but also zookeeper-server for QuorumPeerConfig.parseDynamicConfig to parse quorum members.
zookeeper bundles above modules and keeps compatibility with old zookeeper artifact.

Possible future steps:

Add zookeeper-client-it for client side tests that requiring server functionality.
Migrate zookeeper-cli to dedicated quorum config parse method(ZOOKEEPER-4971) to make it zookeeper-server free. Then, we probably can rename it to zookeeper-client-cli.

eolivelli · 2025-08-24T10:53:16Z

I have been looking forward to this work for long time, amd I had tried in the past.

Is there any recent discussion on the mailing list?

kezhuw · 2025-08-24T12:17:57Z

Is there any recent discussion on the mailing list?

I am dealing with osgi staffs, and planing to start discussion mail soon.

Here is the continue work branch: https://github.com/kezhuw/zookeeper/commits/ZOOKEEPER-233-split-server-jar/

kezhuw · 2025-08-24T17:30:55Z

@eolivelli Discussion mail posted: https://lists.apache.org/thread/j4hg8bh6y3rjg5hlzrtnod52t7xz18hv

…ption` `ZKConfig` is client accessible, it should avoid accessing server side `QuorumPeerConfig`. Changes: 1. Change method and constructor signatures of `ZKConfig` and `ZKClientConfig` to throw `o.a.zookeeper.common.ConfigException`. 3. Throw `QuorumPeerConfig.ConfigException` if it is in classpath. Given above changes, this pr maintain abi compatibility with old releases. **Breaking change**: methods and constructors of `ZKConfig` and `ZKClientConfig` throw `org.apache.zookeeper.common.ConfigException` now but not `QuorumPeerConfig.ConfigException`, so developers have to fix it to compile. This is a small step towards ZOOKEEPER-233. Refs: ZOOKEEPER-233, ZOOKEEPER-835, ZOOKEEPER-842, ZOOKEEPER-4970, ZOOKEEPER-4966.

The resulting `zookeeper-client` module does not depend on server side codes. Refs: ZOOKEEPER-233, ZOOKEEPER-835.

…keeper` The resulting `zookeeper-server` module does not depend on client side codes.

This makes `zookeeper` a bundle module while keep compatiblity with old `zookeeper` module. The resulting `zookeeper-cli` depends maily on `zookeeper-client`. It does depend on `QuorumPeerConfig` from `zookeeper-server` to parse quorum members. But this functionality should be exported by client also as we have `ZooKeeper::getConfig`, see ZOOKEEPER-4971.

ctubbsii

Overall, this is really good progress. I think there may be a few small things to adjust after this. It does lay the groundwork for future work to have independent sealed jars with well-defined APIs exposed via JPMS module descriptors, too.

Aside from the couple of things I found and commented on, there are a few things that I think could be done to clean up the directory structure a bit:

Remove "zookeeper-" from all the module directory names. We're already in the zookeeper directory.
Instead of keeping all modules based at the root of the project checkout, you could place them in a modules/ directory, so they can be easily viewed distinct from the bin/, conf/, tools/, etc.

So, instead of:

.
├── bin
├── conf
├── dev
├── tools
├── zookeeper
├── zookeeper-assembly
├── zookeeper-cli
├── zookeeper-client
├── zookeeper-client-c
├── zookeeper-common
...

it could look more like:

.
├── bin
├── conf
├── dev
├── modules
│   ├── assembly
│   ├── cli
│   ├── client
│   ├── client-c
│   ├── common
...

The POM changes for this would be simple. For the <module>, you just specify the path to the module like <module>modules/assembly</module>

Inside the module's POM files, you specify the <relativePath> to the parent POM (a good practice anyway, even if it's just one level up), as in:

  <parent>
    <groupId>org.apache.zookeeper</groupId>
    <artifactId>parent</artifactId>
    <version>3.10.0-SNAPSHOT</version>
    <relativePath>../../pom.xml</relativePath>
  </parent>

ctubbsii · 2025-09-05T21:22:45Z

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>


This is redundant with line 68 above, where this module has already been added to the normal build.

Fixed it in commit 8e7eb53.

ctubbsii · 2025-09-05T21:28:20Z

zookeeper-assembly/pom.xml

-    <dependency>
-      <groupId>org.apache.zookeeper</groupId>
-      <artifactId>zookeeper-client</artifactId>
-      <version>${project.version}</version>
-      <type>pom</type>
-    </dependency>


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.

List zookeeper-client, zookeeper-server and zookeeper-cli explicitly now in e690829. I think it is ok for zookeeper-common to be included transitively ?

For me, I prefer to have tighter control over what gets included in the assembly, so for Accumulo, we do not include any dependencies in our assembly descriptor transitively, and explicitly add them to the POM (see https://github.com/apache/accumulo/tree/2.1/assemble), using a somewhat convoluted process so we just declare what we want in the POM, and only those things get included.

ZooKeeper isn't doing any of that, so it's probably okay for zookeeper-common to be included transitively, but I would probably include it to try to achieve some consistency.

ctubbsii · 2025-09-05T21:31:34Z

zookeeper-assembly/src/main/assembly/bin-package.xml

+    <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>


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.

ctubbsii · 2025-09-05T21:35:49Z

zookeeper-cli/pom.xml

+      <scope>runtime</scope>
+      <optional>true</optional>


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.

Suggested change

<scope>runtime</scope>

<optional>true</optional>

<scope>test</scope>

In previous discussions, there may have been a consensus to just use the test scope for these. I

I though it was optional runtime in #2155. Also, zookeeper-cli uses org.slf4j.Loggeer explicitly, it has no much differences to other modules. I have no much preference to this, but let's keep it consistent for now.

I think I was remembering conceding that test was fine in my comment #2155 (review) ; however, I don't really feel strongly about it. I agree with you that consistency is the main important thing.

ctubbsii · 2025-09-05T21:44:11Z

zookeeper-client-c/pom.xml

+      <plugin>
+        <artifactId>maven-deploy-plugin</artifactId>
+        <configuration>
+          <!-- this module isn't to be deployed to Maven Central -->
+          <skip>true</skip>
+        </configuration>
+      </plugin>


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/

Good point!

We currently don't have this module published. Let's keep it separately to this and explore possible way to reuse a published native jar.

https://mvnrepository.com/artifact/org.apache.zookeeper/zookeeper-client-c

ctubbsii · 2025-09-05T22:02:54Z

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;


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 😺 )

ctubbsii · 2025-09-05T22:03:25Z

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();


Is this a bug in the master branch right now?

I think so but I can not find a failure ci for now.

ctubbsii · 2025-09-05T22:15:39Z

zookeeper/pom.xml

+      <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>


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:

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.

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.

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.

ctubbsii · 2025-09-05T22:21:50Z

zookeeper/src/main/java/org/apache/zookeeper/osgi/Osgi.java

+ * <p>`maven-bundle-plugin` does not include classes if there is no source code. I have no idea why.
+ *


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.

I have replaced it with package-info.java in 0c53d33.

…plicitly in `zookeeper-assembly`

…-plugin`

kezhuw force-pushed the ZOOKEEPER-233-separated-client-jar branch 4 times, most recently from 6d60f35 to 6f5b154 Compare August 21, 2025 07:09

kezhuw mentioned this pull request Aug 22, 2025

ZOOKEEPER-4970: Deprecate ZKConfig methods which throw QuorumPeerConfig.ConfigException #2309

Merged

kezhuw force-pushed the ZOOKEEPER-233-separated-client-jar branch 5 times, most recently from 48dcf1f to 7de4462 Compare August 24, 2025 09:59

kezhuw force-pushed the ZOOKEEPER-233-separated-client-jar branch from 7de4462 to 6dcbe7c Compare August 24, 2025 16:41

kezhuw mentioned this pull request Aug 25, 2025

ZOOKEEPER-4968: Add interfaces to cover ZooKeeper client operations #2308

Draft

kezhuw force-pushed the ZOOKEEPER-233-separated-client-jar branch 2 times, most recently from 7008b8e to 0781442 Compare August 27, 2025 03:56

kezhuw marked this pull request as ready for review August 27, 2025 04:11

kezhuw requested review from phunt, cnauroth, ztzg, anmolnar, eolivelli, maoling, symat, tisonkun and li4wang August 27, 2025 04:12

kezhuw changed the title ~~ZOOKEEPER-233: Provide separated zookeeper-client~~ ZOOKEEPER-233: Split zookeeper module to provide separated zookeeper-client, zookeeper-server Aug 27, 2025

kezhuw added 2 commits August 31, 2025 00:24

ZOOKEEPER-233: Split zookeeper-client for zookeeper

cc4c376

The resulting `zookeeper-client` module does not depend on server side codes. Refs: ZOOKEEPER-233, ZOOKEEPER-835.

kezhuw added 2 commits August 31, 2025 00:30

ZOOKEEPER-233: Split zookeeper-common, zookeeper-server from `zoo…

8d011b0

…keeper` The resulting `zookeeper-server` module does not depend on client side codes.

kezhuw force-pushed the ZOOKEEPER-233-separated-client-jar branch from 0781442 to da36e01 Compare August 30, 2025 16:32

ctubbsii suggested changes Sep 5, 2025

View reviewed changes

kezhuw added 3 commits September 10, 2025 13:24

zookeeper-cient-c belongs to full-build profile

8e7eb53

Include zookeeper-client, zookeeper-server and zookeeper-cli ex…

e690829

…plicitly in `zookeeper-assembly`

Introduce package-info.java in module zookeeper for `maven-bundle…

0c53d33

…-plugin`

	<scope>runtime</scope>
	<optional>true</optional>
	<scope>test</scope>

		* <p>`maven-bundle-plugin` does not include classes if there is no source code. I have no idea why.
		*

ZOOKEEPER-233: Split zookeeper module to provide separated zookeeper-client, zookeeper-server #2307

Are you sure you want to change the base?

ZOOKEEPER-233: Split zookeeper module to provide separated zookeeper-client, zookeeper-server #2307

Conversation

kezhuw commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli commented Aug 24, 2025

Uh oh!

kezhuw commented Aug 24, 2025

Uh oh!

kezhuw commented Aug 24, 2025

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZOOKEEPER-233: Split `zookeeper` module to provide separated `zookeeper-client`, `zookeeper-server` #2307

ZOOKEEPER-233: Split `zookeeper` module to provide separated `zookeeper-client`, `zookeeper-server` #2307

kezhuw commented Aug 20, 2025 •

edited

Loading