ZOOKEEPER-4835: Make use of Netty optional when no SSL is used by dsmiley · Pull Request #2374 · apache/zookeeper

dsmiley · 2026-04-20T17:39:44Z

https://issues.apache.org/jira/browse/ZOOKEEPER-4835

ZOOKEEPER-4835: Make Netty an optional dependency when SSL is not used

Netty is now declared as <optional>true</optional> in zookeeper-server/pom.xml,
so embedders who do not need SSL/TLS no longer pull in Netty transitively.
The zookeeper-assembly distribution still includes Netty for full SSL support.

Key changes:

Split ClientX509Util into a Netty-free base class and ClientNettyX509Util subclass,
so the common X509/SSL abstraction compiles without Netty on the classpath.
Removed hard Netty references from ZooKeeper, ZooKeeperServer, and startup classes;
Netty factory/util classes are now loaded reflectively or guarded by classpath checks.
Added an ArchUnit test (NettyOptionalArchTest) to enforce that core classes never
acquire a hard compile-time dependency on Netty.

I tested that Solr's tests pass when changing it to use a local snapshot of ZK with these changes. Solr's tests embed ZK server, and also are a client to ZK.

… used (#1) * ZOOKEEPER-4835: Make Netty optional when no SSL is used Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/f61cc9b9-0ac8-4f8f-a366-2a7752cdbedc Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com> * Address code review: improve error messages and add constants for magic numbers Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/f61cc9b9-0ac8-4f8f-a366-2a7752cdbedc Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com> * Remove non-essential log lines; add ArchUnit test to enforce Netty is optional Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/34abe384-a396-4ef6-9ba0-006a7318faa8 Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com> * Refine ArchUnit test using dsmiley's approach; fix remaining Netty hard-refs in ZooKeeper/ZooKeeperServer Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/698a4b0f-1e5b-46f8-bf10-f21f9a65a382 Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com> * Fix NoClassDefFoundError: split ClientX509Util into Netty-free base and ClientNettyX509Util subclass Agent-Logs-Url: https://github.com/dsmiley/zookeeper/sessions/2dc4e1b6-53f1-4851-91ed-b4427d332564 Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dsmiley <377295+dsmiley@users.noreply.github.com>

dsmiley

All code changes were done by GitHub Copilot Agent, and me going through 3-4 rounds of prompts... plus me showing the ArchUnit test I wrote a few years ago.

dsmiley · 2026-04-20T17:41:31Z

All the code here moved from ClientX509Util

PDavid

Disclaimer: I'm not (yet) a ZooKeeper committer so you'll still need ZooKeeper committer approval for merging.

Many thanks, this looks like a nice change. 👍 I added some comments / questions.

PDavid · 2026-05-28T11:22:12Z


            if (config.getSecureClientPortAddress() != null) {
-                secureCnxnFactory = ServerCnxnFactory.createFactory();
+                secureCnxnFactory = ServerCnxnFactory.createFactory(true);


The same issue should also be fixed in QuorumPeer.java:2426. The dynamic reconfiguration path for secureCnxnFactory still calls the no-arg createFactory(), which defaults to NIO. This means adding a secure client port via live reconfig will silently produce a non-TLS server socket.

wow, good catch! I included the simple change, even though it's arguably out-of-scope.

Many thanks for fixing this. 👍

PDavid · 2026-05-28T12:13:36Z

+ * on {@code io.netty} packages. All other ZooKeeper classes must remain Netty-free so
+ * that Netty can be an optional dependency for users who do not need SSL/TLS.
+ */
+public class NettyOptionalArchTest {


I really like that you added this test. 👍

Hexdump: use platform newline

PDavid

Many thanks for the additional improvements you made. This looks good to me. 👍

As I mentioned I'm not a ZooKeeper committer so you'll still need ZooKeeper committer approval for merging.

PDavid

Thanks a lot. 👍

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

dsmiley · 2026-08-13T03:13:57Z

@PDavid could you please consider merging this?

anmolnar · 2026-09-02T15:15:16Z

+        } else if (clientCnxnSocketName.equals("ClientCnxnSocketNetty")) {
+            clientCnxnSocketName = "org.apache.zookeeper.ClientCnxnSocketNetty";


I'm not sure hardcoding the full classname here is the right thing to do. There could be clients where a shaded version of ZooKeeper is being referenced, like in the HBase client, and in that case this hardcoded value won't work or even break compatibility.

Why do you need this change?

"Why" is hopefully clear based on the fundamental point of this issue/PR -- to prevent a dependency on Netty when the user doesn't configure ZK to need it.

Good point about shading. I'll fix in a minute by using the same package as another, thus potentially shade-relative if shading is used.

Oh, thanks, my bad.

anmolnar · 2026-09-02T15:19:59Z

+      <dependency>
+        <groupId>org.mockito</groupId>
+        <artifactId>mockito-inline</artifactId>
+        <version>${mockito.version}</version>
+        <scope>test</scope>
+      </dependency>
+      <dependency>
+        <groupId>com.tngtech.archunit</groupId>
+        <artifactId>archunit-junit5</artifactId>
+        <version>${archunit.version}</version>
+      </dependency>


Mockito scope is "test", but archunit is not. Why is that?
Where do you use mockito-inline in the new code?

LLM generated

The mockito-inline part is a red herring. This PR doesn't add or touch mockito-inline — it's pre-existing in the root . It only shows up in anmolnar's diff view as context lines directly above the new archunit-junit5 entry. The new tests (NettyOptionalArchTest, HexDumpOutputFormatterTest) use no Mockito at all. That's just a PR-comment reply, no code change.

The scope inconsistency is real but minor. The new archunit-junit5 entry in root has no test, while its neighbors mockito-inline and junit-jupiter do. That section is already mixed (mockito-core, junit-vintage-engine have no scope), so it's not a hard rule — but archunit is genuinely test-only (one ArchUnit test, nothing in src/main), and zookeeper-server/pom.xml already declares it test at point of use.

No, mockito-inline doesn't exist in the root pom on the master branch. This patch is about to add it.

Good catch; you are quite right! I must have messed that up from the merge... given this PR has been sitting around forever, it increased the chances of having to update it. Now fixed.

dsmiley commented Apr 20, 2026

View reviewed changes

checkstyle fixes

a1c0fbd

PDavid reviewed May 28, 2026

View reviewed changes

dsmiley added 2 commits May 29, 2026 23:09

code review updates.

272839b

Hexdump: use platform newline

fix format

a199417

PDavid approved these changes Jun 2, 2026

View reviewed changes

Comment thread zookeeper-server/src/test/java/org/apache/zookeeper/cli/HexDumpOutputFormatterTest.java

Comment thread zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxnFactory.java

collapse catch blocks

6f623fd

PDavid approved these changes Jun 3, 2026

View reviewed changes

dsmiley mentioned this pull request Jun 4, 2026

ZOOKEEPER-4324: dropwizard-metrics should be optional #2402

Open

dsmiley and others added 2 commits August 12, 2026 16:56

Merge remote-tracking branch 'apache/master'

ae59aa3

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Resolve apache/master merge for Netty SSLContext supplier

987e761

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

anmolnar requested changes Sep 2, 2026

View reviewed changes

dsmiley added 3 commits September 2, 2026 11:44

Use a shade-compatible reference

2adc65f

pom.xml: explicit on scope=test

44a91ae

pom.xml: remove mockito-inline

9584c69

		} else if (clientCnxnSocketName.equals("ClientCnxnSocketNetty")) {
		clientCnxnSocketName = "org.apache.zookeeper.ClientCnxnSocketNetty";

Uh oh!

Conversation

dsmiley commented Apr 20, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PDavid 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PDavid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PDavid left a comment

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Aug 13, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants