You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2014/06/05 23:07:19 UTC
[jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
If the TemplateBuilderImpl is given an imageId but the image can not be
found in the image cache, fallback to the GetImageStrategy to perform a
call to the provider to try to get it.
We've seen that in some cases images are not returned in the image list
but they actually exist in the provider. This fix won't make them
available when filtering by other properties such as the operating system,
etc, but at least will make them available if their id is known.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds jclouds-570
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/396
-- Commit Summary --
* JCLOUDS-570: Fallback to the GetImageStrategy
-- File Changes --
M apis/cloudsigma/src/main/java/org/jclouds/cloudsigma/compute/CloudSigmaTemplateBuilderImpl.java (5)
M apis/ec2/src/main/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImpl.java (6)
M apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2TemplateBuilderTest.java (7)
M apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java (27)
M apis/vcloud/src/main/java/org/jclouds/vcloud/compute/internal/VCloudTemplateBuilderImpl.java (5)
M compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java (39)
M compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java (244)
M providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2TemplateBuilderImpl.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/396.patch
https://github.com/jclouds/jclouds/pull/396.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> @@ -727,12 +731,21 @@ public Template build() {
> }
>
> private Image findImageWithId(Set<? extends Image> images) {
> - Image image;
> - // TODO: switch to GetImageStrategy in version 1.5
> - image = tryFind(images, idPredicate).orNull();
> - if (image == null)
> - throwNoSuchElementExceptionAfterLoggingImageIds(format("%s not found", idPredicate), images);
> - return image;
> + // Try find the image in the cache and fallback to the GetImageStrategy
[minor] "Try to find..."
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522274
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1190](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1190/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45321554
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> + expect(getImageStrategy.getImage(image.getId())).andReturn(image);
> + replay(getImageStrategy);
> +
> + Provider<TemplateBuilder> templateBuilderProvider = new Provider<TemplateBuilder>() {
> + @Override
> + public TemplateBuilder get() {
> + return createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, this, getImageStrategy);
> + }
> + };
> +
> + // Note that the image provided is not in the image list, but it is the one returned by the GetImagestrategy
> + TemplateBuilder templateBuilder = templateBuilderProvider.get().imageId(image.getId());
> + Template template = templateBuilder.build();
> +
> + assertEquals(template.getImage().getId(), image.getId());
> + verify(getImageStrategy);
Are we interested in how exactly this call is made? We know _some_ call is made because the result is correct...but would we care if e.g. _two_ calls were made?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522291
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> @@ -727,12 +731,21 @@ public Template build() {
> }
>
> private Image findImageWithId(Set<? extends Image> images) {
> - Image image;
> - // TODO: switch to GetImageStrategy in version 1.5
> - image = tryFind(images, idPredicate).orNull();
> - if (image == null)
> - throwNoSuchElementExceptionAfterLoggingImageIds(format("%s not found", idPredicate), images);
> - return image;
> + // Try find the image in the cache and fallback to the GetImageStrategy
> + // see https://issues.apache.org/jira/browse/JCLOUDS-570
> + Optional<? extends Image> image = tryFind(images, idPredicate);
> + if (!image.isPresent()) {
> + logger.warn("Image %s not found in the image cache. Trying to get it directly...", imageId);
> + // Note that this might generate make a call to the provider instead of using a cache, but
[minor] "Note that this will generate a call..."?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522275
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1189](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1189/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45280697
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> + expect(getImageStrategy.getImage(image.getId())).andReturn(image);
> + replay(getImageStrategy);
> +
> + Provider<TemplateBuilder> templateBuilderProvider = new Provider<TemplateBuilder>() {
> + @Override
> + public TemplateBuilder get() {
> + return createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, this, getImageStrategy);
> + }
> + };
> +
> + // Note that the image provided is not in the image list, but it is the one returned by the GetImagestrategy
> + TemplateBuilder templateBuilder = templateBuilderProvider.get().imageId(image.getId());
> + Template template = templateBuilder.build();
> +
> + assertEquals(template.getImage().getId(), image.getId());
> + verify(getImageStrategy);
> and we need to make sure the strategy is not being called again.
OK, then let's leave as-is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13599893
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Ignasi Barrera <no...@github.com>.
> + expect(getImageStrategy.getImage(image.getId())).andReturn(image);
> + replay(getImageStrategy);
> +
> + Provider<TemplateBuilder> templateBuilderProvider = new Provider<TemplateBuilder>() {
> + @Override
> + public TemplateBuilder get() {
> + return createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, this, getImageStrategy);
> + }
> + };
> +
> + // Note that the image provided is not in the image list, but it is the one returned by the GetImagestrategy
> + TemplateBuilder templateBuilder = templateBuilderProvider.get().imageId(image.getId());
> + Template template = templateBuilder.build();
> +
> + assertEquals(template.getImage().getId(), image.getId());
> + verify(getImageStrategy);
Yep, we want to check the exact number of calls, at least for the moment. See comment below about the WIP pull request: there is a test that validates that images have properly been registered in the image cache and we need to make sure the strategy is not being called again.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13599490
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #862](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/862/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45278668
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
Thanks for this, @nacx! A bunch of minor comments added. There's also one [Checkstyle violation](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/862/org.apache.jclouds$jclouds-compute/violations/).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45426285
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> + };
> +
> + final GetImageStrategy getImageStrategy = createMock(GetImageStrategy.class);
> +
> + expect(getImageStrategy.getImage(image.getId())).andReturn(image);
> + replay(getImageStrategy);
> +
> + Provider<TemplateBuilder> templateBuilderProvider = new Provider<TemplateBuilder>() {
> + @Override
> + public TemplateBuilder get() {
> + return createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, this, getImageStrategy);
> + }
> + };
> +
> + // Note that the image provided is not in the image list, but it is the one returned by the GetImagestrategy
> + TemplateBuilder templateBuilder = templateBuilderProvider.get().imageId(image.getId());
I'm probably missing something here...why do we need to construct the complicated templateBuliderProvider above if we simply call `get()` on it here? Can't we just use `createTemplateBuilder(...)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522294
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #863](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/863/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45320609
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Ignasi Barrera <no...@github.com>.
> + };
> +
> + final GetImageStrategy getImageStrategy = createMock(GetImageStrategy.class);
> +
> + expect(getImageStrategy.getImage(image.getId())).andReturn(image);
> + replay(getImageStrategy);
> +
> + Provider<TemplateBuilder> templateBuilderProvider = new Provider<TemplateBuilder>() {
> + @Override
> + public TemplateBuilder get() {
> + return createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, this, getImageStrategy);
> + }
> + };
> +
> + // Note that the image provided is not in the image list, but it is the one returned by the GetImagestrategy
> + TemplateBuilder templateBuilder = templateBuilderProvider.get().imageId(image.getId());
One of the arguments to create the template builder is a template builder provider (note the *this* parameter). That's why the provider is used instead of directly creating it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13599356
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1333](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1333/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45278844
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Ignasi Barrera <no...@github.com>.
Closed #396.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#event-128755653
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> @@ -133,13 +136,14 @@
> protected TemplateBuilderImpl(@Memoized Supplier<Set<? extends Location>> locations,
> @Memoized Supplier<Set<? extends Image>> images, @Memoized Supplier<Set<? extends Hardware>> hardwares,
> Supplier<Location> defaultLocation2, @Named("DEFAULT") Provider<TemplateOptions> optionsProvider,
> - @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider) {
> - this.locations = locations;
> - this.images = images;
> - this.hardwares = hardwares;
> - this.defaultLocation = defaultLocation2;
> - this.optionsProvider = optionsProvider;
> - this.defaultTemplateProvider = defaultTemplateProvider;
> + @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider, GetImageStrategy getImageStrategy) {
> + this.locations = checkNotNull(locations, "locations");
> + this.images = checkNotNull(images, "locations");
> + this.hardwares = checkNotNull(hardwares, "hardwares");
> + this.defaultLocation = checkNotNull(defaultLocation2, "defaultLocation2");
Why "defaultLocation2", by the way?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522272
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> + };
> +
> + final GetImageStrategy getImageStrategy = createMock(GetImageStrategy.class);
> +
> + expect(getImageStrategy.getImage(image.getId())).andReturn(image);
> + replay(getImageStrategy);
> +
> + Provider<TemplateBuilder> templateBuilderProvider = new Provider<TemplateBuilder>() {
> + @Override
> + public TemplateBuilder get() {
> + return createTemplateBuilder(null, locations, images, hardwares, region, optionsProvider, this, getImageStrategy);
> + }
> + };
> +
> + // Note that the image provided is not in the image list, but it is the one returned by the GetImagestrategy
> + TemplateBuilder templateBuilder = templateBuilderProvider.get().imageId(image.getId());
> (note the this parameter)
Ah, I had missed that. Thanks for clarifying!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13599866
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Ignasi Barrera <no...@github.com>.
I've addressed all comments in an upcoming pull request, that fixes [JCLOUDS-588](https://issues.apache.org/jira/browse/JCLOUDS-588). It is not opened yet (I want to validate it with the reporter of the issue), but you can have a first look at [my branch](https://github.com/nacx/jclouds/tree/588-imagecache).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45627478
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> @@ -179,9 +184,10 @@ public void testParseOnDemandWithoutRegionEncodedIntoId() {
> replay(defaultOptions);
> replay(optionsProvider);
> replay(templateBuilderProvider);
> + replay(getImageStrategy);
[minor] Turn these into `replay(defaultOptions, optionsProvider, ...)`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13473984
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> @@ -192,6 +198,7 @@ public void testParseOnDemandWithoutRegionEncodedIntoId() {
> verify(defaultOptions);
> verify(optionsProvider);
> verify(templateBuilderProvider);
> + verify(getImageStrategy);
[minor] Turn these into `verify(defaultOptions, optionsProvider, ...)`, assuming we really want to verify the calls here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13473989
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1334](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1334/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45320747
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> @@ -133,13 +136,14 @@
> protected TemplateBuilderImpl(@Memoized Supplier<Set<? extends Location>> locations,
> @Memoized Supplier<Set<? extends Image>> images, @Memoized Supplier<Set<? extends Hardware>> hardwares,
> Supplier<Location> defaultLocation2, @Named("DEFAULT") Provider<TemplateOptions> optionsProvider,
> - @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider) {
> - this.locations = locations;
> - this.images = images;
> - this.hardwares = hardwares;
> - this.defaultLocation = defaultLocation2;
> - this.optionsProvider = optionsProvider;
> - this.defaultTemplateProvider = defaultTemplateProvider;
> + @Named("DEFAULT") Provider<TemplateBuilder> defaultTemplateProvider, GetImageStrategy getImageStrategy) {
> + this.locations = checkNotNull(locations, "locations");
> + this.images = checkNotNull(images, "locations");
"images"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522270
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [1.7.x](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=ad79081f433293bb0ca658e7eeb37e0354bc885e) too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45352728
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> }
>
> + // The EC2 provider already overrides the getImage method in a way that this test makes no sense in it.
"The EC2 provider already overrides the getImage method so this test is not useful for EC2" or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13475523
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Andrew Phillips <no...@github.com>.
> - if (image == null)
> - throwNoSuchElementExceptionAfterLoggingImageIds(format("%s not found", idPredicate), images);
> - return image;
> + // Try find the image in the cache and fallback to the GetImageStrategy
> + // see https://issues.apache.org/jira/browse/JCLOUDS-570
> + Optional<? extends Image> image = tryFind(images, idPredicate);
> + if (!image.isPresent()) {
> + logger.warn("Image %s not found in the image cache. Trying to get it directly...", imageId);
> + // Note that this might generate make a call to the provider instead of using a cache, but
> + // this will be executed rarely, only when an image is not present in the image list but
> + // it actually exists in the provider. It shouldn't be an expensive call so using a cache just for
> + // this corner case is overkill.
> + image = Optional.fromNullable(getImageStrategy.getImage(imageId));
> + if (!image.isPresent()) {
> + throwNoSuchElementExceptionAfterLoggingImageIds(format("%s not found", idPredicate), images);
> + }
[minor] We can save a bit of object construction at the cost of a little more complexity:
```
ImageThing imageFromProvider = getImageStrategy.getImage(imageId);
if (imageFromProvider == null) {
...
}
image = Optional.of(imageFromProvider); // or just return imageFromProvider
```
What do you think?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396/files#r13522288
Re: [jclouds] JCLOUDS-570: Fallback to the GetImageStrategy in
TemplateBuilderImpl (#396)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=39f77ad3f80efd7416f17a204aaa7b5b79629aa3) (I've also checked that the downstream repos are not broken by this change). Will backport to 1.7.x later today.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/396#issuecomment-45325079