You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Oliver Gondža <no...@github.com> on 2015/07/16 23:16:40 UTC
[jclouds] JCLOUDS-962: Do not override provider methods (#818)
[JCLOUDS-962](https://issues.apache.org/jira/browse/JCLOUDS-962)
Fix guice 4 support.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/818
-- Commit Summary --
* JCLOUDS-962: Do not override provider methods
-- File Changes --
M apis/byon/src/main/java/org/jclouds/byon/config/BYONComputeServiceContextModule.java (21)
M apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/config/CloudStackComputeServiceContextModule.java (6)
M apis/ec2/src/main/java/org/jclouds/ec2/compute/config/EC2ComputeServiceContextModule.java (14)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/config/NovaComputeServiceContextModule.java (4)
M compute/src/main/java/org/jclouds/compute/config/BaseComputeServiceContextModule.java (8)
M compute/src/main/java/org/jclouds/compute/stub/config/StubComputeServiceContextModule.java (13)
M core/src/main/java/org/jclouds/logging/config/ConsoleLoggingModule.java (6)
M core/src/main/java/org/jclouds/logging/config/LoggingModule.java (5)
M core/src/main/java/org/jclouds/logging/config/NullLoggingModule.java (6)
M core/src/main/java/org/jclouds/logging/jdk/config/JDKLoggingModule.java (5)
M drivers/log4j/src/main/java/org/jclouds/logging/log4j/config/Log4JLoggingModule.java (5)
M drivers/slf4j/src/main/java/org/jclouds/logging/slf4j/config/SLF4JLoggingModule.java (5)
M providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/config/AWSEC2ComputeServiceContextModule.java (4)
M providers/glesys/src/main/java/org/jclouds/glesys/compute/config/GleSYSComputeServiceContextModule.java (20)
M providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java (41)
M skeletons/standalone-compute/src/main/java/org/jclouds/servermanager/compute/config/ServerManagerComputeServiceContextModule.java (21)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/818.patch
https://github.com/jclouds/jclouds/pull/818.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Also, I'll setup a Jenkins build that uses Guice 4 so we can use it to track the status of this issue.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122118982
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Andrew Phillips <no...@github.com>.
Created a new matrix build to test this PR against Guice 3.0 and 4.0:
https://jclouds.ci.cloudbees.com/job/jclouds-guice-3-4/
Thanks, @olivergondza and @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-123081488
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Agree.
I've already configured a build that uses Guice 4.0 to track this pull request: https://jclouds.ci.cloudbees.com/view/public/job/jclouds-guice-4/
Let's see if it is automatically triggered and reports feedback here: rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122206718
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Yep, build is passing: https://jclouds.ci.cloudbees.com/view/public/job/jclouds-guice-4/37/
Regarding the final methods, clients can expect to break when upgrading to 2.0.0; we just need to make sure there is a way for them to recover. It would be ideal to have all provider methods implement the pattern that allows inheritance, but I think that would introduce a lot of unnecessary boilerplate code, as most of those provider methods (if not all) are unlikely to be overridden, so I'd say it is ok to leave them final.
I'm ok with merging this as-is. It allows us to support Guice 4.0 now and will give users enough time to plan the migration and open any issues they find, but I'd like to have some feedback from the community too, so I'd leave the PR open a couple days and share this in the ML. Sounds good?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-123046610
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
As soon as this gets merged the 2.0.0-SNAPSHOT will be automatically released to the Apache snapshots repo, so you'll be able to start testing it (see http://jclouds.apache.org/start/install/).
A formal release in Apache is a bit more complicated as the release has to be proposed and voted by the community first. If the snapshot approach is not an option I'd encourage you to start a guice4-friendly release preview discussion in the jclouds dev@ mailing list.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-123197621
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for looking at this @olivergondza!
Looking at the change, I'd suggest a different approach: what about leaving the provider annotations in the parent class, make the method `final` to make sure it is not overridden my mistake, and make it call another protected method that actually does the work? This way we can leave the guice configuration to the parent class and keep the current behavior. This should be less error-prone regarding Guice misconfigurations. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122116143
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Squashed the fix commit and pushed to master. Thanks @olivergondza and apologies for the delay! I've also updated the Jenkins build to build the master branch with both versions of Guice.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-137265896
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Oliver Gondža <no...@github.com>.
Alright, I have marked all provider methods final to identify the overridden ones and created a delegate where needed. Still, there is a lot of methods that _was not_ overriden and thus have no delegate methods. I am undecided if we should keep them `final` to identify problems in dependent libraries or revert back to non-final not to break the clients.
This patch now works for me.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122243714
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Closed #818.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#event-399512874
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Oliver Gondža <no...@github.com>.
@nacx, Looking back at the amount of boilerplate this generates, keeping the `@Provider` annotation in parent class is a must. Refactoring:
```java
@Provide
public X provideX() { ... }
```
into
```java
@Provide
public final X guiceProvideX() { ... }
public X provideX() { ... }
```
should not break clients and limit the scope of the change to parent classes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122164856
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
@demobox good to merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-125159476
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
Looking a bit deeper, and to make sure we don't allow mistakes in Guice 4, I think the right approach would be to make **all** provider methods in the code base `final`, even if they are still not subclassed, to avoid failures in the future.
Running the following command will help identifying all provider methods defined in the module classes:
```bash
find . -name '*.java' -exec grep '@Provides' -nH -A3 {} \; | grep -i module
```
WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122117633
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Ignasi Barrera <no...@github.com>.
rebuild please
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-122123042
Re: [jclouds] JCLOUDS-962: Do not override provider methods (#818)
Posted by Oliver Gondža <no...@github.com>.
Would it be possible to publish alpha/rc with the announcement so people can start to experiment and provide more useful feedback? It is not a deal breaker but it blocks https://github.com/jenkinsci/openstack-cloud-plugin/pull/15, and the release would help.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/818#issuecomment-123186000