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/09/21 17:08:15 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #12119: [Discovery Service] Remove module and all its references
michaeljmarshall opened a new pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119
### Motivation
As discussed on the [dev](https://lists.apache.org/x/thread.html/rc8b796dda2bc1b022e855c7368d832b570967cb1ef29e9cd18e04e97@%3Cdev.pulsar.apache.org%3E) and [users](https://lists.apache.org/x/thread.html/rc30a4699a1040e6613adbd3eced218f838844ff7f5ef9713ee11be96@%3Cusers.pulsar.apache.org%3E) mailing lists, the Pulsar Discovery Service no longer appears necessary.
As further justification, when inspecting maven central, the discovery service jar is only a dependency for other Pulsar modules, which only use the discovery service for tests. https://mvnrepository.com/artifact/org.apache.pulsar/pulsar-discovery-service/usages
This PR removes the Pulsar Discovery Service. It leaves old references to the discovery service in the versioned docs, as the discovery service is still available for those versions of Pulsar.
### Modifications
* Remove `pulsar-discovery-service` module
* Remove discovery references from `bin/pulsar`
* Remove discovery references from `bin/pulsar-daemon`
* Remove `conf/discovery.conf` file
* Remove all references to `pulsar-discovery-service` module from `pom.xml` files
* Remove discovery service tests from the `pulsar-broker` module (only removed tests for the discovery service module)
* Update docs
### Verifying this change
I built the project and will ensure tests pass. The one concern might be that I've deleted too many tests. I only deleted tests with explicit references to the removed module. It'd be worth a second check from someone else, too.
### 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: no
- 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: yes
I've described the changes above. We're removing a module, so it will break certain parts of our API. The module appears unused, and we've taken steps to discuss this on the mailing list to ensure there is proper discussion and notice.
### Documentation
I've updated the docs by removing references to the discovery service. It'd be a good idea to make sure this gets mentioned in the change log when we release the major version that includes this change.
--
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 commented on a change in pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#discussion_r714628721
##########
File path: distribution/server/pom.xml
##########
@@ -160,6 +154,16 @@
<artifactId>simpleclient_log4j2</artifactId>
</dependency>
+ <!-- This dependency was previously brought in transitively through the pulsar-discovery-service module. When the
Review comment:
we do not need this comment in the source code
--
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] merlimat merged pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119
--
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] michaeljmarshall commented on pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#issuecomment-926096050
@eolivelli - makes sense to me. I am fine merging this before we cut branch-2.9.
Regarding the comment in the pom.xml, which I removed in my most recent commit, I want to make sure that adding the bouncy castle dependency to the server pom.xml is the correct solution. The discovery service was the dependency forcing our version of bouncy castle to 1.69. Without the discovery service module as a dependency, the `grpc-xds` dependency determined out bouncy castle version, which would have been 1.61. Since we need the `grpc-xds` dependency, and we want version 1.69 for security reasons, I added the dependency to the server's pom.xml.
--
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 commented on pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#issuecomment-925765087
if this is useless it is better to drop it in a major release like 2.9.
We can always revert the deletion if someone cares about this module (it looks like no one responded to the ML yet)
--
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] merlimat commented on pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#issuecomment-926217461
> @eolivelli - makes sense to me. I am fine merging this before we cut branch-2.9.
>
> Regarding the comment in the pom.xml, which I removed in my most recent commit, I want to make sure that adding the bouncy castle dependency to the server pom.xml is the correct solution. The discovery service was the dependency forcing our version of bouncy castle to 1.69. Without the discovery service module as a dependency, the `grpc-xds` dependency determined out bouncy castle version, which would have been 1.61. Since we need the `grpc-xds` dependency, and we want version 1.69 for security reasons, I added the dependency to the server's pom.xml.
Yes, I think that was the correct fix. Merging this.
--
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] michaeljmarshall commented on pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#issuecomment-925073190
@jiazhai - I notice that you wrote the documentation on Bouncy Castle dependencies. Would you be able to take a look at this PR to verify that I've followed the right paradigm for using the right version of Bouncy Castle? Because the dependency on `bouncy-castle-bc` was removed with the `pulsar-discovery-service` module, the pulsar server did not have any explicit dependency bringing in `bouncy-castle-bc`. I've added it explicitly in my most recent commit. Without this declared dependency, the bouncy castle deps are brought in as transitive dependencies.
--
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] michaeljmarshall commented on pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#issuecomment-925358389
/pulsarbot run-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
[GitHub] [pulsar] michaeljmarshall commented on pull request #12119: [Discovery Service] Remove module and all its references
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12119:
URL: https://github.com/apache/pulsar/pull/12119#issuecomment-924330167
Looks like bouncy castle dependencies/licenses are making the "misc" tests fail. I think part of it is that the discovery module was the only module that required `org.apache.pulsar:bouncy-castle-bc:jar:pkg:2.9.0-SNAPSHOT:complie`. When running `mvn dependency:tree` on both `master` and on my branch.
The error message for the test is:
```
$ src/check-binary-license ./distribution/server/target/apache-pulsar-*-bin.tar.gz
org.bouncycastle-bcpkix-jdk15on-1.61.jar unaccounted for in LICENSE
org.bouncycastle-bcprov-jdk15on-1.61.jar unaccounted for in LICENSE
org.bouncycastle-bcpkix-jdk15on-1.69.jar mentioned in LICENSE, but not bundled
org.bouncycastle-bcprov-jdk15on-1.69.jar mentioned in LICENSE, but not bundled
org.bouncycastle-bcutil-jdk15on-1.69.jar mentioned in LICENSE, but not bundled
```
When I inspect the server distribution after building from this branch, I see the following:
```
$ tar -tf apache-pulsar-2.9.0-SNAPSHOT-bin.tar.gz | grep -i bounc
apache-pulsar-2.9.0-SNAPSHOT/licenses/LICENSE-bouncycastle.txt
apache-pulsar-2.9.0-SNAPSHOT/lib/presto/plugin/pulsar-presto-connector/bouncy-castle-bc-2.9.0-SNAPSHOT-pkg.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.apache.pulsar-bouncy-castle-bc-2.9.0-SNAPSHOT-pkg.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.bouncycastle-bcprov-ext-jdk15on-1.69.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.bouncycastle-bcpkix-jdk15on-1.61.jar
apache-pulsar-2.9.0-SNAPSHOT/lib/org.bouncycastle-bcprov-jdk15on-1.61.jar
```
It looks like the 1.61 jars are coming from a grpc dependency. After running `mvn dependency:tree`, I can see the following several times throughout our dependency tree:
```
[INFO] | | | \- io.grpc:grpc-xds:jar:1.33.0:test
[INFO] | | | +- org.bouncycastle:bcpkix-jdk15on:jar:1.61:test
[INFO] | | | | \- org.bouncycastle:bcprov-jdk15on:jar:1.61:test
[INFO] | | | \- io.grpc:grpc-netty-shaded:jar:1.33.0:test (version selected from constraint [1.33.0,1.33.0])
```
I'm not familiar enough with Maven or with our build to know the right way to solve this. On one hand, `grpc-xds` is bringing in an older version of bouncy castle, which is known to have security issues (https://github.com/apache/pulsar/pull/10867), so I think we'll want to force the version to 1.69. Note that the latest version of grpc-xds is 1.40.1 and is only using bouncy castle jars [1.67](https://github.com/grpc/grpc-java/blob/v1.40.1/build.gradle#L184). On the other hand, is it a good idea to be using a later version of bouncy castle than the `grpc-xds` jar requires?
@lhotari - can you help me figure out the right next step here?
--
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