Improved pagination in GCE #62
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,8 +119,7 @@ public Set<SecurityGroup> listSecurityGroupsForNode(String id) { | |
| return ImmutableSet.of(); | ||
| } | ||
|
|
||
| ImmutableSet.Builder<SecurityGroup> builder = ImmutableSet.builder(); | ||
|
Contributor
There was a problem hiding this comment. Another thing that is fine, but I'd say in the future we should stop Set'ing everything. List is fine for things like this. |
||
|
|
||
| for (NetworkInterface nwInterface : instance.getNetworkInterfaces()) { | ||
| String networkUrl = nwInterface.getNetwork().getPath(); | ||
|
|
@@ -171,7 +170,7 @@ public boolean removeSecurityGroup(String id) { | |
|
|
||
| ListOptions options = new ListOptions.Builder().filter("network eq .*/" + id); | ||
|
|
||
| FluentIterable<Firewall> fws = api.getFirewallApiForProject(userProject.get()).list(options).toPagedIterable().concat(); | ||
|
Contributor
There was a problem hiding this comment. I'd prefer
Contributor
Author
There was a problem hiding this comment. Do you mean this? FluentIterable.from(api.getFirewallApiForProject(userProject.get())
.list(options)
.toPagedIterable()
.concat());The PagedIterable#concat already returns a FluentIterable, so that won't be needed (and that method in Guava is deprecated). Are you in the direction of changing the |
||
|
|
||
| for (Firewall fw : fws) { | ||
| AtomicReference<Operation> operation = Atomics.newReference(api.getFirewallApiForProject(userProject.get()) | ||
|
|
@@ -203,7 +202,8 @@ public SecurityGroup addIpPermission(IpPermission ipPermission, SecurityGroup gr | |
|
|
||
| ListOptions options = new ListOptions.Builder().filter("network eq .*/" + group.getName()); | ||
|
|
||
| FluentIterable<Firewall> fws = api.getFirewallApiForProject(userProject.get()).list(options).toPagedIterable().concat(); | ||
| if (fws.anyMatch(providesIpPermission(ipPermission))) { | ||
| // Permission already exists. | ||
| return group; | ||
| } | ||
|
|
@@ -268,7 +268,7 @@ public SecurityGroup removeIpPermission(IpPermission ipPermission, SecurityGroup | |
|
|
||
| ListOptions options = new ListOptions.Builder().filter("network eq .*/" + group.getName()); | ||
|
|
||
| FluentIterable<Firewall> fws = api.getFirewallApiForProject(userProject.get()).list(options).toPagedIterable().concat(); | ||
|
|
||
| for (Firewall fw : fws) { | ||
| if (equalsIpPermission(ipPermission).apply(fw)) { | ||
|
|
@@ -328,7 +328,7 @@ public boolean supportsExclusionCidrBlocks() { | |
|
|
||
| private SecurityGroup groupForTagsInNetwork(Network nw, final Set <String> tags) { | ||
| ListOptions opts = new Builder().filter("network eq .*/" + nw.getName()); | ||
| Set<Firewall> fws = api.getFirewallApiForProject(userProject.get()).list(opts).toPagedIterable().concat() | ||
| .filter(new Predicate<Firewall>() { | ||
| @Override | ||
| public boolean apply(final Firewall input) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,120 +16,59 @@ | |
| */ | ||
| package org.jclouds.googlecomputeengine.domain; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkNotNull; | ||
|
|
||
| import java.util.Iterator; | ||
|
|
||
| import org.jclouds.collect.IterableWithMarker; | ||
| import org.jclouds.collect.IterableWithMarkers; | ||
| import org.jclouds.collect.PagedIterable; | ||
| import org.jclouds.collect.PagedIterables; | ||
| import org.jclouds.googlecomputeengine.domain.Resource.Kind; | ||
|
|
||
| import com.google.common.base.Function; | ||
| import com.google.common.base.Optional; | ||
| import com.google.common.collect.ImmutableSet; | ||
|
|
||
| /** | ||
| * A single page returned from a paginated collection that knows how to advance | ||
| * to the next page. | ||
| */ | ||
| public class ListPage<T> extends IterableWithMarker<T> { | ||
|
|
||
| private final PageWithMarker<T> delegate; | ||
| private final Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction; | ||
|
|
||
| public ListPage(PageWithMarker<T> delegate, Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction) { | ||
|
Contributor
There was a problem hiding this comment. prefer to separate listpage so that it can be a simple data type. offering an additional method or otherwise mean to construct an advancing list would be more ideal than having all lists have additional internal state that includes references to jclouds context internals. For example, a reference to this list will keep a lot of a jclouds context from being gc'd, and the user may not suspect that. I'm not saying we don't already do things like this. Just suggesting that today, not sure we would always prefer to conflate the list with its advancing function for all use cases. Simple way out is to make an alternate method that returns a normal list.
Contributor
Author
There was a problem hiding this comment. If we want to be able to build a PagedIterable given single page (IterableWithMarker or ListPage) we need the advancing function. Wether it is already set inside the list object or we use an alternate method to build it (such as PagedIterables#advance) we need to hold a reference to that function as long as the List object exists; otherwise it won't be able to lazily fetch new pages as they are needed. This said, I tried to hide the advancing function from users because it might be difficult to get:
That made me go for the Just to make sure I understand your concern: we could be leaking the reference to the advancing function only in the case the user does not want to advance to the next page. When getting the "entire" list provided by the PagedIterable, the advancing function is always needed. IMO, the former use case might not be the most used one, as the method that returns the ListPage is the one used when filtering resources or performing searches, but that's only a guess; we really don't know which of both methods will be used more. I see two options to move forward:
Am I getting the picture? WDYT?
Contributor
There was a problem hiding this comment. Will followup more completely later.. maybe :) since maybe I will answer One thing to keep in mind that in many cases there will be only one page of It may be possible in the current design to special case this. Also we had a meetup with clojure folks back in the day (specifically toni In all, I think this change is a step backwards or at best neutral, from a I am not focused on it or the several other patterns we use, and admit that In the mean time, feel free to merge this, taking into consideration what
Contributor
Author
There was a problem hiding this comment. In fact, I first implemented it by returning a regular Anyway, I'm OK to change to that approach, but I'll also refactor the majority of the tests. Most of the expect tests are a mess, and live tests that use lists have a lot of improvement, so I'll change them and refactor all expect tests to MWS ones. It will take some more time, but we'll have a cleaner provider and the tests in the way we want them :) Thanks for the feedback!
Contributor
There was a problem hiding this comment. no problemo. I like that you are thinking about things bigger than (but |
||
| this.delegate = checkNotNull(delegate, "delegate"); | ||
| this.advancingFunction = checkNotNull(advancingFunction, "advancingFunction"); | ||
| } | ||
|
|
||
| public PagedIterable<T> toPagedIterable() { | ||
| return advancingFunction.apply(delegate); | ||
| } | ||
|
|
||
| @Override | ||
| public Iterator<T> iterator() { | ||
| return delegate.iterator(); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<Object> nextMarker() { | ||
| return delegate.nextMarker(); | ||
| } | ||
|
|
||
| public static class Empty<T> extends ListPage<T> { | ||
|
|
||
| public Empty(Kind kind) { | ||
| super(PageWithMarker.<T>builder().kind(kind).build(), new Function<PageWithMarker<T>, PagedIterable<T>>() { | ||
| @Override | ||
| public PagedIterable<T> apply(PageWithMarker<T> input) { | ||
| return PagedIterables.onlyPage(IterableWithMarkers.from(ImmutableSet.<T>of())); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git seems to know something I don't know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes :) I removed the PATCH class as it already exists in jclouds-core. That seems to have fooled git.