You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/01/06 15:36:46 UTC
[GitHub] [pulsar] janssk1 opened a new pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
janssk1 opened a new pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140
Fixes #7971
### Motivation
Since pulsar 2.6, the client is not useable anymore in projects that use jdk modules. The jars files contain split packages, preventing the jvm from starting
### Modifications
Two 'split packages' fixed:
- ZStd compression was reusing packages of airlift to access a protected class. Using reflection instead
- Changed package of LongSchemaVersion from org.apache.pulsar.common.schema to org.apache.pulsar.common.protocol.schema. The former package is used already by client-api
### Verifying this change
- *Added integration tests that runs using java modules. Unfortunately, a maven limitation (https://issues.apache.org/jira/browse/MNG-5899) prevents me from adding the test to the reactor. For now, the test needs to be executed in a dedicated mvn call..*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: **yes --> LongSchemaVersion package changed. Not sure if this is a public API **
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no
### Documentation
- Does this pull request introduce a new feature? no
- If a feature is not applicable for documentation, explain why? Bugfix
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#discussion_r552797289
##########
File path: pulsar-client-modules-test/pom.xml
##########
@@ -0,0 +1,84 @@
+<!--
+
+ 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.
+
+-->
+<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/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.pulsar</groupId>
+ <artifactId>pulsar</artifactId>
+ <version>2.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <artifactId>pulsar-client-modules-test</artifactId>
+
+ <properties>
+ <maven.compiler.source>11</maven.compiler.source>
+ <maven.compiler.target>11</maven.compiler.target>
+ </properties>
+
+ <dependencies>
+ <dependency>
+ <groupId>${project.groupId}</groupId>
+ <artifactId>pulsar-client</artifactId>
+ <version>${project.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
+ <version>1.7.30</version>
Review comment:
```suggestion
<version>${slf4j.version}</version>
```
##########
File path: pulsar-client-modules-test/pom.xml
##########
@@ -0,0 +1,84 @@
+<!--
+
+ 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.
+
+-->
+<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/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.pulsar</groupId>
+ <artifactId>pulsar</artifactId>
+ <version>2.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <artifactId>pulsar-client-modules-test</artifactId>
+
+ <properties>
+ <maven.compiler.source>11</maven.compiler.source>
Review comment:
This will not compile on Java 8, this module should only be enabled on Java11+
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#issuecomment-1058894255
The pr had no activity for 30 days, mark with Stale label.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] fransguelinckx commented on a change in pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
fransguelinckx commented on a change in pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#discussion_r588243732
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/compression/ZStdRawCompressor.java
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.compression;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+/**
+ * Expose ZstdFrameCompressor which is a package protected class.
+ */
+public class ZStdRawCompressor {
+
+ private final static Method compressMethod;
+
+ static {
+ try {
+ Class<?> frameCompressor = ZStdRawCompressor.class.getClassLoader()
+ .loadClass("io.airlift.compress.zstd.ZstdFrameCompressor");
+ compressMethod = frameCompressor.getDeclaredMethod("compress", Object.class, long.class, long.class,
+ Object.class, long.class, long.class, int.class);
+ } catch (ClassNotFoundException | NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static int compress(long inputAddress, long inputLimit,
+ long outputAddress, long outputLimit, int compressionLevel) {
+ try {
+ return (int) compressMethod.invoke(null, null, inputAddress,
Review comment:
In what direction were you thinking when you said 'playing with packages'? I don't see a way out at the moment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] janssk1 commented on a change in pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
janssk1 commented on a change in pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#discussion_r552873054
##########
File path: pulsar-client-modules-test/pom.xml
##########
@@ -0,0 +1,84 @@
+<!--
+
+ 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.
+
+-->
+<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/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.pulsar</groupId>
+ <artifactId>pulsar</artifactId>
+ <version>2.8.0-SNAPSHOT</version>
+ <relativePath>..</relativePath>
+ </parent>
+
+ <artifactId>pulsar-client-modules-test</artifactId>
+
+ <properties>
+ <maven.compiler.source>11</maven.compiler.source>
Review comment:
Right. As mentioned in the pull request, it does not even build in the same reactor due to a mvn issue. So this test must run in another mvn invocation as the normal build. Not sure how to set that up in pulsar..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#issuecomment-1058897609
@janssk1:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] janssk1 commented on pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
janssk1 commented on pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#issuecomment-759657041
@merlimat I added a github workflow to run the modules test using java 11. That will prevent other split package situations in the future. Let me know if anything else is needed in order to get this merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#discussion_r588275125
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/compression/ZStdRawCompressor.java
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.compression;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+/**
+ * Expose ZstdFrameCompressor which is a package protected class.
+ */
+public class ZStdRawCompressor {
+
+ private final static Method compressMethod;
+
+ static {
+ try {
+ Class<?> frameCompressor = ZStdRawCompressor.class.getClassLoader()
+ .loadClass("io.airlift.compress.zstd.ZstdFrameCompressor");
+ compressMethod = frameCompressor.getDeclaredMethod("compress", Object.class, long.class, long.class,
+ Object.class, long.class, long.class, int.class);
+ } catch (ClassNotFoundException | NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static int compress(long inputAddress, long inputLimit,
+ long outputAddress, long outputLimit, int compressionLevel) {
+ try {
+ return (int) compressMethod.invoke(null, null, inputAddress,
Review comment:
I took a look deeper
It looks like at airlift they are not happy to make the class public
https://github.com/airlift/aircompressor/pull/101
I wonder if we can cheat the system (at least while working in classpath mode) and created dynamically the io.airlift.compress.zstd.ZStdRawDecompressor class in the right package.
You will also need to add an interface in order to be able to call the method as you cannot bind directly to io.airlift.compress.zstd.ZStdRawDecompressor at compile time.
The complexity of this solution is not trivial, so probably the best to do is to create some little JHM benchmark and understand how much we are going to pay for using reflection before spending time.
Did you evaluate to use MethodHandles as well ?
That's my humble opinion, I am not asking you to spend more and more time on this work,
let's wait for feedback from the others in the community
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] fransguelinckx commented on a change in pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
fransguelinckx commented on a change in pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#discussion_r588243732
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/compression/ZStdRawCompressor.java
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.compression;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+/**
+ * Expose ZstdFrameCompressor which is a package protected class.
+ */
+public class ZStdRawCompressor {
+
+ private final static Method compressMethod;
+
+ static {
+ try {
+ Class<?> frameCompressor = ZStdRawCompressor.class.getClassLoader()
+ .loadClass("io.airlift.compress.zstd.ZstdFrameCompressor");
+ compressMethod = frameCompressor.getDeclaredMethod("compress", Object.class, long.class, long.class,
+ Object.class, long.class, long.class, int.class);
+ } catch (ClassNotFoundException | NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static int compress(long inputAddress, long inputLimit,
+ long outputAddress, long outputLimit, int compressionLevel) {
+ try {
+ return (int) compressMethod.invoke(null, null, inputAddress,
Review comment:
In what direction were you thinking when you say 'playing with packages'? I don't see a way out at the moment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9140: [Issue #7971] [pulsar-client] Make pulsar-client usable through java-modules
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9140:
URL: https://github.com/apache/pulsar/pull/9140#discussion_r588201178
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/compression/ZStdRawCompressor.java
##########
@@ -0,0 +1,53 @@
+/**
+ * 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.pulsar.common.compression;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+/**
+ * Expose ZstdFrameCompressor which is a package protected class.
+ */
+public class ZStdRawCompressor {
+
+ private final static Method compressMethod;
+
+ static {
+ try {
+ Class<?> frameCompressor = ZStdRawCompressor.class.getClassLoader()
+ .loadClass("io.airlift.compress.zstd.ZstdFrameCompressor");
+ compressMethod = frameCompressor.getDeclaredMethod("compress", Object.class, long.class, long.class,
+ Object.class, long.class, long.class, int.class);
+ } catch (ClassNotFoundException | NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static int compress(long inputAddress, long inputLimit,
+ long outputAddress, long outputLimit, int compressionLevel) {
+ try {
+ return (int) compressMethod.invoke(null, null, inputAddress,
Review comment:
Using reflection will affect performances, do you have numbers to about the impact ?
isn't there any other way, by playing with package names ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org