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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
