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/11 14:37:07 UTC
[jclouds] JCLOUDS-588: Register discovered images in the image cache
(#401)
https://issues.apache.org/jira/browse/JCLOUDS-588
Images were cached in memory using a memoized supplier. To allow growing this cache with the discovered images, the `ImageCacheSupplier` class has been created. It provides an in-memory cache with all discovered images and acts as a view over the image cache that also provides access to them.
The in-memory cache for the discovered images expires with the session, just as the image cache does.
The default memoized image supplier has been changed to the `ImageCacheSupplier`, to make sure all providers get injected the right instance, and the old supplier has been qualified with the 'imageCache' name, in case a provider needs the basic image cache.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds 588-imagecache
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/401
-- Commit Summary --
* JCLOUDS-588: Register discovered images in the image cache
-- File Changes --
M apis/cloudsigma/src/main/java/org/jclouds/cloudsigma/compute/CloudSigmaTemplateBuilderImpl.java (4)
M apis/ec2/src/main/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImpl.java (3)
M apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2TemplateBuilderTest.java (3)
M apis/ec2/src/test/java/org/jclouds/ec2/compute/internal/EC2TemplateBuilderImplTest.java (11)
M apis/vcloud/src/main/java/org/jclouds/vcloud/compute/internal/VCloudTemplateBuilderImpl.java (4)
M compute/src/main/java/org/jclouds/compute/config/BaseComputeServiceContextModule.java (6)
M compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java (40)
M compute/src/main/java/org/jclouds/compute/strategy/impl/AdaptingComputeServiceStrategies.java (9)
A compute/src/main/java/org/jclouds/compute/suppliers/ImageCacheSupplier.java (84)
M compute/src/test/java/org/jclouds/compute/domain/internal/TemplateBuilderImplTest.java (80)
A compute/src/test/java/org/jclouds/compute/suppliers/ImageCacheSupplierTest.java (77)
M providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2TemplateBuilderImpl.java (3)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/401.patch
https://github.com/jclouds/jclouds/pull/401.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1348](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1348/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401#issuecomment-45738418
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Everett Toews <no...@github.com>.
> // 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);
> - }
> + if (image.isPresent()) {
> + return image.get();
> + }
> +
> + logger.warn("Image %s not found in the image cache. Trying to get it from the provider...", imageId);
There needs to be another log message after the image has been fetched confirming that it was fetched and cached. If I saw only the message above in a log, it would look like an error to me because it's "Trying..." but never succeeds?
Or you could just move the log message to after the image fetch and change the message to be in the past tense. e.g. ""Image %s not found in the image cache. Image was fetch from the provider and cached in jclouds."
Also, should this be a debug statement?
It's not exactly unexpected behaviour. I'm not sure. I could go either way.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401/files#r13655674
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1229](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1229/) 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/401#issuecomment-46156781
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Ignasi Barrera <no...@github.com>.
> // 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);
> - }
> + if (image.isPresent()) {
> + return image.get();
> + }
> +
> + logger.warn("Image %s not found in the image cache. Trying to get it from the provider...", imageId);
I like the past tense approach after having fetched the image. Will change.
Regarding the log level, I'd rather use warning, as using this "hidden" images is something we might want to notify users (and we can expect debug level to be disabled). Take into account that the message will appear only once, since the image is being added to the cache. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401/files#r13656806
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1364](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1364/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401#issuecomment-46156579
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Andrew Phillips <no...@github.com>.
> // 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);
> - }
> + if (image.isPresent()) {
> + return image.get();
> + }
> +
> + logger.warn("Image %s not found in the image cache. Trying to get it from the provider...", imageId);
> Regarding the log level, I'd rather use warning, as using this "hidden" images is something we might want to notify
How about INFO, then? I'd agree that this is something users might want to know about (i.e. level > DEBUG), but it's not a *problem* as such - you wouldn't expect to trigger an alert based on this, for example (i.e. level < WARN)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401/files#r13738427
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #893](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/893/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401#issuecomment-46156416
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #877](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/877/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401#issuecomment-45738254
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Everett Toews <no...@github.com>.
> // 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);
> - }
> + if (image.isPresent()) {
> + return image.get();
> + }
> +
> + logger.warn("Image %s not found in the image cache. Trying to get it from the provider...", imageId);
Works for me.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401/files#r13658638
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1211](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1211/) 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/401#issuecomment-45739582
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Everett Toews <no...@github.com>.
> // 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);
> - }
> + if (image.isPresent()) {
> + return image.get();
> + }
> +
> + logger.warn("Image %s not found in the image cache. Trying to get it from the provider...", imageId);
That's a good point. WARN indicates something is slightly off but if a user is putting in the id directly, this is expected behaviour.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401/files#r13755496
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Ignasi Barrera <no...@github.com>.
Squashed, rebased, and pushed to [master]8https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=b0250cfb4f7adcb8ed9ecb77bd8228d5f415bb50) and [1.7.x](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=3589c1475aa919e5a874b7082af748425a6b97b4)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401#issuecomment-46678790
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Ignasi Barrera <no...@github.com>.
Closed #401.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401#event-133508049
Re: [jclouds] JCLOUDS-588: Register discovered images in the image
cache (#401)
Posted by Ignasi Barrera <no...@github.com>.
> // 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);
> - }
> + if (image.isPresent()) {
> + return image.get();
> + }
> +
> + logger.warn("Image %s not found in the image cache. Trying to get it from the provider...", imageId);
Agree :) Changed!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/401/files#r13795234