You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2020/03/26 10:42:58 UTC
[GitHub] [curator] bigmarvin opened a new pull request #355: [CURATOR-464]
attach shaded artifacts with classifier uber
bigmarvin opened a new pull request #355: [CURATOR-464] attach shaded artifacts with classifier uber
URL: https://github.com/apache/curator/pull/355
Would like to revisit the following issue.
https://issues.apache.org/jira/browse/CURATOR-464
The current approach would be preventing shaded artifacts from replacing original artifacts. Instead, shaded artifacts are now also attached with classifier uber, next to original artifacts, so clients could integrate either based on their need.
This approach is my preference while it'll be a little expensive in adoption, as classifier must be added. If this really bothers, here are 2 alternatives we may go:
1. **Removal of reloaction in shading.** Packages like `org.apache.curator.shaded.com.google` could be found nowhere but in curator-client, which however doesn't export these relocated packages in its manifest. Without relocation, these packages could be fulfilled by some other bundle, following the import instruction inside the manifest. However, if shaded classes take priority, problems could be expected in OSGi runtime because same classes are loaded by different bundles and they become different classses.
2. **Attaching original artifact with some classifier like 'slim'.** This is essentially the reverse way of the proposed approach, which however costs little in adoption as no classifier is required. Personally, I don't like this way as we don't usually put classifier on original artifacts. However, if compared with this issue not being fixed, this alternative would be better.
Please review the change and any comment is welcome.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on issue #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
Randgalt commented on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-615949082
Sorry - I've been tied up. I'll look at this soon.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative #2, I propose a new change where projects of type jar or bundle would have original artifacts attached, with classifier 'original'. It's better than 'slim', as shading does more than simply including. The byte code of curator classes is also changed during shading, e.g. imports of google guava packages become imports of relocated guava packages.
Though listed in supported types in the bundle plugin, projects of type war are not included in this change, because those projects simply packs everything in the dependency tree into artifacts.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative #2, I propose a new change where projects of type jar or bundle would have original artifacts attached, with classifier 'original'. It's better than 'slim', as shading does more than simply including. The byte code of curator classes is also changed during shading, e.g. imports of google guava packages become imports of relocated guava packages.
Though listed in supported types in the bundle plugin, projects of type war are not included in this change, because those projects simply packs everything in the dependency tree into artifacts, and they don't have separation in class loading.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin commented on issue #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin commented on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the name and the content of this PR.
Following alternative #2, I propose a new change where projects of type jar or bundle would have original artifacts attached. In this way, existing clients could remain enjoying shaded artifacts during upgrade, and new clients may choose either based on their need.
Though listed in supported types of bundle plugin, projects of type war are not included in this change, because those projects simply packs everything in the dependency tree into artifacts, and they don't have separation in class loading.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative 2, I propose a new change where projects of type jar or bundle would have original artifacts attached, with classifier 'original'. It's better than 'slim', as shading does more than simply including. The byte code of curator classes is also modified during shading, e.g. imports of google guava packages become imports of relocated guava packages.
Though listed in supported types in the bundle plugin, projects of type war are not included in this change, because those projects simply pack everything in the dependency tree into artifacts.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin closed pull request #355: [CURATOR-464] attach
shaded artifacts with classifier uber
Posted by GitBox <gi...@apache.org>.
bigmarvin closed pull request #355: [CURATOR-464] attach shaded artifacts with classifier uber
URL: https://github.com/apache/curator/pull/355
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin commented on issue #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin commented on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-608529754
May I ask for some review and comments?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] Randgalt edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
Randgalt edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-615951286
A few things:
- Why do we need ant and build helper plugins. Can't `install-file` from the install plugin be used?
- If I understand the need correctly this only needs to be done on the `curator-client` module right? That's where the shading happens. Or is that you need versions of the other modules that don't refer to the shaded Guava? Will that actually work? That code is all compiled assuming a particular version of Guava. I guess if OSGI-Curator users use the right version of Guava it would work.
- I'm not convinced that `original` is the right prefix for the unshaded JAR. Is there a standard prefix for these things? I'd appreciate it if you'd search around and show some other projects that are doing this and show what prefixes they use.
- We should update the website docs (see <root>/src/site) to detail these additional JARs. That should be part of this PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin commented on issue #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin commented on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-612894834
@Randgalt, may I ask for your review around 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative #2, I propose a new change where projects of type jar or bundle would have original artifacts attached, with classifier 'original'. It's better than 'slim', as shading does more than simply including. The byte code of curator classes is also changed during shading, e.g. imports of google guava packages become imports of relocated guava packages.
Though listed in supported types in the bundle plugin, projects of type war are not included in this change, because those projects simply packs everything in the dependency tree into artifacts, and they don't have isolation in class loading.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
Randgalt edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-615951286
A few things:
- Why do we need ant and build helper plugins. Can't `install-file` from the install plugin be used?
- If I understand the need correctly this only needs to be done on the `curator-client` module right? That's where the shading happens. Or is that you need versions of the other modules that don't refer to the shaded Guava? Will that actually work? That code is all compiled assuming a particular version of Guava. I guess if OSGI-Curator users use the right version of Guava it would work.
- I'm not convinced that `original` is the right prefix for the unshaded JAR. Is there a standard prefix for these things? I'd appreciate it if you'd search around and show some other projects that are doing this and show what prefixes they use.
- We should update the website docs (see `<root>/src/site`) to detail these additional JARs. That should be part of this PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin opened a new pull request #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin opened a new pull request #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355
Would like to revisit the following issue.
https://issues.apache.org/jira/browse/CURATOR-464
The current approach would be preventing shaded artifacts from replacing original artifacts. Instead, shaded artifacts are now also attached with classifier uber, next to original artifacts, so clients could integrate either based on their need.
This approach is my preference while it'll be a little expensive in adoption, as new classifier must be added during upgrade. If this really bothers, here are 2 alternatives we may go:
1. **Removing the reloaction in shading.** Packages like `org.apache.curator.shaded.com.google` could be found nowhere but in curator-client, which doesn't export these relocated packages in its manifest. Without relocation, these packages could be fulfilled by some other bundle, following the import instruction inside the manifest. However, if shaded classes take priority, problems would be expected in OSGi runtime because same classes are loaded by different bundles and they become different classses. This decision belongs to the implementation of OSGi runtime.
2. **Attaching original artifact with some classifier like 'slim'.** This is essentially the reverse way of the proposed approach, which costs little in adoption as no new classifier is required during upgrade. Personally, I don't like this way as we don't usually put classifier on original artifacts. However, if compared with this issue not being fixed, this alternative would be better.
Please review the change and any comment is welcome.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative 2, I propose a new change where projects of type jar or bundle would have original artifacts attached, with classifier 'original'. It's better than 'slim', as shading does more than simply including. The byte code of curator classes is also changed during shading, e.g. imports of google guava packages become imports of relocated guava packages.
Though listed in supported types in the bundle plugin, projects of type war are not included in this change, because those projects simply pack everything in the dependency tree into artifacts.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on issue #355: [CURATOR-464] attach
shaded artifacts with classifier uber
Posted by GitBox <gi...@apache.org>.
Randgalt commented on issue #355: [CURATOR-464] attach shaded artifacts with classifier uber
URL: https://github.com/apache/curator/pull/355#issuecomment-604541395
I'm -1 on this PR. The vast majority don't use OSGI and don't want to have to manage a separate artifact. If I understand alternative 2, publish a new artifact without the shaded code, I'd be OK with that.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on issue #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
Randgalt commented on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-616170464
I don't understand how this got merged - did I do it by accident? I didn't intend to. I'm going to open another PR to revert changes so we can continue the discussion.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] asfgit merged pull request #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative #2, I propose a new change where projects of type jar or bundle would have original artifacts attached, with classifier 'original'. It's better than 'slim', as shading does more than simply including. The byte code of curator classes is also changed during shading, e.g. imports of google guava packages become imports of relocated guava packages.
Though listed in supported types in the bundle plugin, projects of type war are not included in this change, because those projects simply pack everything in the dependency tree into artifacts.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] bigmarvin edited a comment on issue #355: [CURATOR-464]
attach orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
bigmarvin edited a comment on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-604842044
Updating the title and the commit of this PR.
Following alternative #2, I propose a new change where projects of type jar or bundle would have original artifacts attached. In this way, existing clients could remain enjoying shaded artifacts during upgrade, and new clients may choose either based on their need.
Though listed in supported types of bundle plugin, projects of type war are not included in this change, because those projects simply packs everything in the dependency tree into artifacts, and they don't have separation in class loading.
Projects of type jar, on the other hand, could produce artifacts usable as bundles, by planting headers required by OSGi runtime into manifest during packaging, so they are included.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on issue #355: [CURATOR-464] attach
orignal artifacts with classifier original
Posted by GitBox <gi...@apache.org>.
Randgalt commented on issue #355: [CURATOR-464] attach orignal artifacts with classifier original
URL: https://github.com/apache/curator/pull/355#issuecomment-615951286
A few things:
- Why do we need ant and build helper plugins. Can't `install-file` from the install plugin be used?
- If I understand the need correctly this only needs to be done on the `curator-client` module right? That's where the shading happens. Or is that you need versions of the other modules that don't refer to the shaded Guava? Will that actually work? That code is all compiled assuming a particular version of Guava. I guess if OSGI-Curator users use the right version of Guava it would work.
- I'm not convinced that `original` is the right prefix for the unshaded JAR. Is there a standard prefix for these things? I'd appreciate it if you'd search around and show some other projects that are doing this and show what prefixes they use.
----------------------------------------------------------------
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
With regards,
Apache Git Services