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/10/23 01:57:01 UTC
[jclouds-labs-google] Improved pagination in GCE (#62)
This pull requests improves the pagination in GCE so it no longer sucks :D The main changes are:
* Removed all `listFirstPage` and similar methods from all APIs.
* All *list* methods have two forms: one to list all records that returns a `PagedIterable` (no params), and one to get a concrete page that accepts a `ListOptions` object.
* The `ListPage` object representing a single page has been refactored and improved and now has a convenience method `toPagedIterable`, that allows converting it to iterate over the pages after a filter operation or similar.
* Refactored all parsers to accommodate the new pagination logic.
In summary, every API only has the minimum and meaningful methods to get a list of records, and the pagination domain objects have been reworked to make it trivial to iterate over the pages.
I've also removed most of the warnings int he project.
/cc @ccustine @danbroudy Could you give a try to the branch and run the live tests? Your feedback on how the new pagination methods look like from a user point of view is appreciated too :)
/cc @adriancole Your feedback is appreciated too.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds-labs-google pagination
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/62
-- Commit Summary --
* Improved pagination
-- File Changes --
R google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/GoogleComputeEngineFallbacks.java (30)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (3)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/extensions/GoogleComputeEngineSecurityGroupExtension.java (12)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/BuildInstanceMetadata.java (2)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/FirewallToIpPermission.java (2)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/InstanceInZoneToNodeMetadata.java (10)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/NetworkToSecurityGroup.java (14)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/options/GoogleComputeEngineTemplateOptions.java (1)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/ListPage.java (115)
A google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/PageWithMarker.java (135)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/AddressApi.java (58)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/DiskApi.java (60)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/DiskTypeApi.java (61)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/FirewallApi.java (65)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/GlobalOperationApi.java (65)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ImageApi.java (64)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/InstanceApi.java (67)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/MachineTypeApi.java (65)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/NetworkApi.java (65)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/RegionApi.java (59)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/RegionOperationApi.java (68)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/RouteApi.java (59)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/SnapshotApi.java (57)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ZoneApi.java (59)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ZoneOperationApi.java (68)
A google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BasePageParser.java (61)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BaseToPagedIterable.java (8)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BaseWithRegionToPagedIterable.java (8)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BaseWithZoneToPagedIterable.java (8)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseAddresses.java (26)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseDiskTypes.java (22)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseDisks.java (26)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseFirewalls.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseGlobalOperations.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseImages.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseInstances.java (28)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseMachineTypes.java (25)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseNetworks.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseRegionOperations.java (26)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseRegions.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseRoutes.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseSnapshots.java (25)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseZoneOperations.java (26)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseZones.java (24)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/options/ListOptions.java (17)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/PageSystemExpectTest.java (14)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/functions/InstanceInZoneToNodeMetadataTest.java (15)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/functions/NetworkToSecurityGroupTest.java (21)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/AddressApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/AddressApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskTypeApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskTypeApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/FirewallApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/FirewallApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/GlobalOperationApiExpectTest.java (12)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/GlobalOperationApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ImageApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ImageApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiLiveTest.java (3)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/MachineTypeApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/MachineTypeApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/NetworkApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/NetworkApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionOperationApiExpectTest.java (30)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionOperationApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RouteApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RouteApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/SnapshotApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/SnapshotApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneApiExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneOperationApiExpectTest.java (24)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneOperationApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseAddressListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseDiskListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseDiskTypeListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseFirewallListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseImageListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseInstanceListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseMachineTypeListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseNetworkListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseOperationListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseRegionListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseRouteListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseSnapshotListTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseZoneListTest.java (8)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/62.patch
https://github.com/jclouds/jclouds-labs-google/pull/62.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
>
> - @ConstructorProperties({ "kind", "nextPageToken", "items" })
> - protected ListPage(Kind kind, String nextPageToken, Iterable<T> items) {
> - this.kind = checkNotNull(kind, "kind");
> - this.nextPageToken = nextPageToken;
> - this.items = items != null ? ImmutableList.copyOf(items) : ImmutableList.<T>of();
> + public ListPage(PageWithMarker<T> delegate, Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction) {
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](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/collect/PagedIterables.java#L68-L75)) 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:
* Users calling a list method would need some deeper knowledge on how it works internally to be able to pick the right function.
* When directly building the Api you don't have access to the Injector (however, if you build a Context such as the compute one, you have the `utils().injector()` method), so there might not be an easy way to get a proper instance the right function to use.
That made me go for the `ListPage` approach having the `toPagedIterable` method and already having a reference to the advancing function.
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:
* Keep the ListPage as-is, in favor of api usage simplicity, but with the possibility of holding an unneeded reference to the advancing function.
* Get rid of that ListPage and make the api methods just return a regular `IterableWithMarker`. This will make users have to use the existing `PagedIterables.advance` to build a Pagediterable, and manually get the advancing function by some mean.
Am I getting the picture? WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19264065
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -393,7 +393,8 @@ public void suspendNode(String name) {
> private LoginCredentials getFromImageAndOverrideIfRequired(org.jclouds.compute.domain.Image image,
> GoogleComputeEngineTemplateOptions options) {
> LoginCredentials defaultCredentials = image.getDefaultCredentials();
> - String[] keys = defaultCredentials.getPrivateKey().split(":");
> + // Atthis point the private key must exist
> + String[] keys = defaultCredentials.getOptionalPrivateKey().get().split(":");
Agree. My intent was only to remove the warnings, but not start a major cleanup of the provider, but I'd be happy to start a global cleanup PR in the provider to address all things we want to improve.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19263291
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
Closed #62.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62#event-188092276
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
> +import static org.jclouds.googlecomputeengine.domain.Resource.Kind;
> +
> +import java.beans.ConstructorProperties;
> +import java.util.Iterator;
> +
> +import org.jclouds.collect.IterableWithMarker;
> +
> +import com.google.common.base.MoreObjects;
> +import com.google.common.base.Objects;
> +import com.google.common.base.Optional;
> +import com.google.common.collect.ImmutableList;
> +
> +/**
> + * A single page returned from a paginated list call.
> + */
> +public class PageWithMarker<T> extends IterableWithMarker<T> {
looks like this used to be called ListPage, right?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19256146
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #173](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/173/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62#issuecomment-60174654
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
Will reopen again after all the cleanup has been done :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62#issuecomment-61659326
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
> @@ -393,7 +393,8 @@ public void suspendNode(String name) {
> private LoginCredentials getFromImageAndOverrideIfRequired(org.jclouds.compute.domain.Image image,
> GoogleComputeEngineTemplateOptions options) {
> LoginCredentials defaultCredentials = image.getDefaultCredentials();
> - String[] keys = defaultCredentials.getPrivateKey().split(":");
> + // Atthis point the private key must exist
> + String[] keys = defaultCredentials.getOptionalPrivateKey().get().split(":");
This is fine, but I will sidebar: particularly with java8 having optional, I feel we should divest or at least stop proliferating this type. Nullable is fine. I don't want to get to a very long debate about it, just raising my opinion on the matter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19255781
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
> +import static org.jclouds.googlecomputeengine.domain.Resource.Kind;
> +
> +import java.beans.ConstructorProperties;
> +import java.util.Iterator;
> +
> +import org.jclouds.collect.IterableWithMarker;
> +
> +import com.google.common.base.MoreObjects;
> +import com.google.common.base.Objects;
> +import com.google.common.base.Optional;
> +import com.google.common.collect.ImmutableList;
> +
> +/**
> + * A single page returned from a paginated list call.
> + */
> +public class PageWithMarker<T> extends IterableWithMarker<T> {
Yep! I preferred to rename this one to keep the signatures of the API as unchanged as possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19264094
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
> @@ -119,8 +119,7 @@ public GoogleComputeEngineSecurityGroupExtension(GoogleComputeEngineApi api,
> return ImmutableSet.of();
> }
>
> - ImmutableSet.Builder builder = ImmutableSet.builder();
> -
> + ImmutableSet.Builder<SecurityGroup> builder = ImmutableSet.builder();
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.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19255842
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -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).concat();
> + FluentIterable<Firewall> fws = api.getFirewallApiForProject(userProject.get()).list(options).toPagedIterable().concat();
Do you mean this?
```java
FluentIterable.from(api.getFirewallApiForProject(userProject.get())
.list(options)
.toPagedIterable()
.concat());
```
The [PagedIterable#concat](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/collect/PagedIterable.java#L73) 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 `PagedIterable.concat()` method to have a regular `Iterable` in the signature? Or am I missing something?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19263534
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
>
> - @ConstructorProperties({ "kind", "nextPageToken", "items" })
> - protected ListPage(Kind kind, String nextPageToken, Iterable<T> items) {
> - this.kind = checkNotNull(kind, "kind");
> - this.nextPageToken = nextPageToken;
> - this.items = items != null ? ImmutableList.copyOf(items) : ImmutableList.<T>of();
> + public ListPage(PageWithMarker<T> delegate, Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction) {
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.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19256126
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
> @@ -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).concat();
> + FluentIterable<Firewall> fws = api.getFirewallApiForProject(userProject.get()).list(options).toPagedIterable().concat();
I'd prefer `FluentIterable.from(api...concat());` or `FluentIterable.from(api...flatten());` I think we should stop proliferating FluentIterable as it can easily wrap things and reduce the surface area we have wrt guava compatibility. (again. not interested in an angry debate with people, just offering advice).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19255921
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
>
> - @ConstructorProperties({ "kind", "nextPageToken", "items" })
> - protected ListPage(Kind kind, String nextPageToken, Iterable<T> items) {
> - this.kind = checkNotNull(kind, "kind");
> - this.nextPageToken = nextPageToken;
> - this.items = items != null ? ImmutableList.copyOf(items) : ImmutableList.<T>of();
> + public ListPage(PageWithMarker<T> delegate, Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction) {
no problemo. I like that you are thinking about things bigger than (but
also including) maintenance drift. Thanks for that!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19342263
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
>
> -/**
git seems to know something I don't know!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19255757
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-labs-google #1516](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1516/) 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-labs-google/pull/62#issuecomment-60174945
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
>
> -/**
Yes :) I removed the PATCH class as it already exists in jclouds-core. That seems to have fooled git.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19263236
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
>
> - @ConstructorProperties({ "kind", "nextPageToken", "items" })
> - protected ListPage(Kind kind, String nextPageToken, Iterable<T> items) {
> - this.kind = checkNotNull(kind, "kind");
> - this.nextPageToken = nextPageToken;
> - this.items = items != null ? ImmutableList.copyOf(items) : ImmutableList.<T>of();
> + public ListPage(PageWithMarker<T> delegate, Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction) {
Will followup more completely later.. maybe :) since maybe I will answer
best now.
One thing to keep in mind that in many cases there will be only one page of
results. In this case the leak risk, as well the bloated class is
unnecessary.
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
batchelli) who specifically asked to not have auto advancing mandatory as
it introduces risk of crossing network boundaries on simple iteration. They
preferred to manage their own pagination.
In all, I think this change is a step backwards or at best neutral, from a
user pov. It Is less copy paste from implementer pov.
I am not focused on it or the several other patterns we use, and admit that
we can defer a better list for a new version (maybe v2)
In the mean time, feel free to merge this, taking into consideration what
you feel is most time and version relevant. We can always copy paste these
methods back.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19288954
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
ps hold on this. let's address it after auto-value applies?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62#issuecomment-61285424
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Adrian Cole <no...@github.com>.
I think I can summarize what you are doing here.
You want to define the list method once.
That makes sense.
Maybe there's another way we can address this which doesn't leak references for simple case.
For example (excuse the bad name)
```java
Lister<Poo> pooLister = api.list();
ListWithMarker<Poo> firstPageOfPoo = pooLister.first();
ListWithMarker<Poo> nextPageOfPoo = pooLister.next(firstPageOfPoo.marker());
Iterable<Poo> flowingPoo = pooLister.continuation();
Subscription<Poo> whoahRx = pooLister.subscribe(pooObserver);
```
In this case, the special casing is in the `Lister` type, or better named decouples us. For the simple case, you can call first() and not leak references out. Not saying we support rx or streams in the future, but that support would be bound to the `Lister` type and could much more easily deal with platform differences inheritance of an Iterable type.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62#issuecomment-60184177
Re: [jclouds-labs-google] Improved pagination in GCE (#62)
Posted by Ignasi Barrera <no...@github.com>.
>
> - @ConstructorProperties({ "kind", "nextPageToken", "items" })
> - protected ListPage(Kind kind, String nextPageToken, Iterable<T> items) {
> - this.kind = checkNotNull(kind, "kind");
> - this.nextPageToken = nextPageToken;
> - this.items = items != null ? ImmutableList.copyOf(items) : ImmutableList.<T>of();
> + public ListPage(PageWithMarker<T> delegate, Function<PageWithMarker<T>, PagedIterable<T>> advancingFunction) {
In fact, I first implemented it by returning a regular `IterableWithMarker` (the usual approach without references to the advancing function in the iterable itself), but I when I started modifying the live and expect tests I decided to change that, as I found it really ugly to keep the current tests with the new approach. All rely in the PagedIterable even when getting one page, and the code turned non-readable when having to manually get the advancing function.
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!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/62/files#r19332948