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 2022/09/27 12:08:38 UTC
[GitHub] [pulsar] nicoloboschi opened a new pull request, #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
nicoloboschi opened a new pull request, #17855:
URL: https://github.com/apache/pulsar/pull/17855
### Motivation
https://github.com/apache/pulsar/pull/17198 added the `pulsar-broker-common` dependency to the `pulsar-client` module. This is very bad because in this way user applications will download the pulsar broker internals dependency and they may start using unsupported API.
Also this breaks the JDK 8 compatibility for the client. This looks like is no more covered by the CI and I will address it in another pull.
### Modifications
* Moved BaseGenerateDocumentation to the pulsar-common module
* Moved source code compatibility to JDK8
* Replaced FieldContext usage with reflections because it's no more accessible
- [x] `doc-not-needed`
--
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] lhotari commented on a diff in pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17855:
URL: https://github.com/apache/pulsar/pull/17855#discussion_r982430000
##########
pulsar-client-all/pom.xml:
##########
@@ -397,6 +397,33 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <version>${maven-enforcer-plugin.version}</version>
+ <executions>
+ <execution>
+ <id>enforce-bytecode-version</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <enforceBytecodeVersion>
+ <maxJdkVersion>1.8</maxJdkVersion>
+ </enforceBytecodeVersion>
+ </rules>
+ </configuration>
+ </execution>
+ </executions>
+ <dependencies>
+ <dependency>
+ <groupId>org.codehaus.mojo</groupId>
+ <artifactId>extra-enforcer-rules</artifactId>
+ <version>1.6.1</version>
Review Comment:
Manage version in the same way as other dependency versions?
--
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] eolivelli merged pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #17855:
URL: https://github.com/apache/pulsar/pull/17855
--
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] lhotari commented on pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #17855:
URL: https://github.com/apache/pulsar/pull/17855#issuecomment-1259747051
I'd suggest applying https://www.mojohaus.org/extra-enforcer-rules/enforceBytecodeVersion.html to prevent future regressions.
--
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] nicoloboschi commented on pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on PR #17855:
URL: https://github.com/apache/pulsar/pull/17855#issuecomment-1260881382
@lhotari I added the enforcer, it was a great suggestion.
Thanks to it, I found out that also `pulsar-package-core` is included in `pulsar-admin-client` dependency, since 2020. (I guess Pulsar 2.6). So we cannot remove the package without a breaking change. We could eventually move used classes to the `pulsar-client-admin-api` module. If a user inadvertly imported some internal classes from `pulsar-package-core`, they will get errors during the client upgrade. However I think it's fine to change it starting from 2.12.0
--
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] nicoloboschi closed pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
nicoloboschi closed pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
URL: https://github.com/apache/pulsar/pull/17855
--
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] lhotari commented on a diff in pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17855:
URL: https://github.com/apache/pulsar/pull/17855#discussion_r982429633
##########
pulsar-client-all/pom.xml:
##########
@@ -397,6 +397,33 @@
</execution>
</executions>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-enforcer-plugin</artifactId>
+ <version>${maven-enforcer-plugin.version}</version>
+ <executions>
+ <execution>
+ <id>enforce-bytecode-version</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <configuration>
+ <rules>
+ <enforceBytecodeVersion>
+ <maxJdkVersion>1.8</maxJdkVersion>
Review Comment:
I guess it would be consistent to use the `pulsar.client.compiler.release` property
```suggestion
<maxJdkVersion>${pulsar.client.compiler.release}</maxJdkVersion>
```
--
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] nicoloboschi commented on pull request #17855: [fix] Remove pulsar-broker-common dependency from pulsar-client
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on PR #17855:
URL: https://github.com/apache/pulsar/pull/17855#issuecomment-1263330648
/pulsarbot rerun-failure-checks
--
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