You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Adrian Cole <no...@github.com> on 2014/11/04 20:17:45 UTC
[jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
As discussed with @nacx PaginatedIterable doesn't add much value and instead requires us to extend a class vs implement an interface. Iterator already has a contract that can define a continuation of pages.
You can merge this Pull Request by running:
git pull https://github.com/adriancole/jclouds-labs-google adrian.listpage-iterator
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/78
-- Commit Summary --
* Migrate off PaginatedIterable to Iterator.
-- File Changes --
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/GoogleComputeEngineFallbacks.java (11)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineService.java (21)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (81)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java (5)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/extensions/GoogleComputeEngineSecurityGroupExtension.java (25)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/NetworkToSecurityGroup.java (4)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/AddressApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/DiskApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/DiskTypeApi.java (18)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/FirewallApi.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ForwardingRuleApi.java (10)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/GlobalOperationApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/HttpHealthCheckApi.java (11)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ImageApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/InstanceApi.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/MachineTypeApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/NetworkApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/RegionApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/RegionOperationApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/RouteApi.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/SnapshotApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/TargetPoolApi.java (10)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ZoneApi.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ZoneOperationApi.java (17)
A google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/AdvancingIterator.java (48)
R google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BaseToIteratorOfListPage.java (20)
R google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BaseWithRegionToIteratorOfListPage.java (25)
R google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/BaseWithZoneToIteratorOfListPage.java (26)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseAddresses.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseDiskTypes.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseDisks.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseFirewalls.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseForwardingRules.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseGlobalOperations.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseHttpHealthChecks.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseImages.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseInstances.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseMachineTypes.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseNetworks.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseRegionOperations.java (17)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseRegions.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseRoutes.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseSnapshots.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseTargetPools.java (15)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseZoneOperations.java (16)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/functions/internal/ParseZones.java (15)
R google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/internal/ListPages.java (30)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/PageSystemExpectTest.java (21)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceExpectTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/functions/NetworkToSecurityGroupTest.java (8)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/AddressApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/AddressApiLiveTest.java (13)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiLiveTest.java (12)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskTypeApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskTypeApiLiveTest.java (20)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/FirewallApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/FirewallApiLiveTest.java (9)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ForwardingRuleApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/GlobalOperationApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/GlobalOperationApiLiveTest.java (22)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/HttpHealthCheckApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ImageApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ImageApiLiveTest.java (19)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiLiveTest.java (17)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/MachineTypeApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/MachineTypeApiLiveTest.java (20)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/NetworkApiExpectTest.java (16)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/NetworkApiLiveTest.java (12)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionApiLiveTest.java (19)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionOperationApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RegionOperationApiLiveTest.java (22)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RouteApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/RouteApiLiveTest.java (13)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/SnapshotApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/SnapshotApiLiveTest.java (12)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/TargetPoolApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/TargetPoolApiLiveTest.java (7)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneApiExpectTest.java (4)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneApiLiveTest.java (18)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneOperationApiExpectTest.java (15)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ZoneOperationApiLiveTest.java (22)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/78.patch
https://github.com/jclouds/jclouds-labs-google/pull/78.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1626](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1626/) 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/78#issuecomment-61700895
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Ignasi Barrera <no...@github.com>.
Nice! Sounds good to me.
My concerns about redundant methods were because in (all?) other providers we have only two methods, and the method that receives the options parameter (with the marker and pagination stuff) returns just the `IterableWithMarker`, which is kinda a normal one. That's why I initially saw the method as redundant.
There are actually the two options: keep the three methods, or make the method that receives the options return a single page, not a "magic iterable" (as other providers do). The latter, though, motivated the discussion in my PR about users having to know how to build/get the advancing function vs the premature optimization of a specialised list.
The actual plan with the three methods (and removing the token parameter) LGTM. I'm just rambling on to give the context to understand my comments, that are based on what I found when thinking about this in the other PR :)
Thx for taking this!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61727059
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #219](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/219/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61697852
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
ps I agree that it is weird to have marker and also have marker in list options! we should remove that... ps. it is token, not marker :)
Here's what I'd suggest (after we refactor out the zone params, etc):
```bash
ListPage<Instance> listPage(token, options); // this allows users to have a non-magical list, and us to make our Iterator functions not self-referencing. For example, users can build on this without being forced to hold a magic iterator.
Iterator<ListPage<Instance>> list();
Iterator<ListPage<Instance>> list(options);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61724684
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
@nacx Addressed. Next step is to dedupe all those region/zone args out of the feature apis.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61716655
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1624](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1624/) 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/78#issuecomment-61699049
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #222](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/222/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61716763
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
Closed #78.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#event-188347888
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
ps test results are ok. they are not better or worse.
```
Failed tests:
GoogleComputeEngineServiceLiveTest>BaseComputeServiceLiveTest.testOptionToNotBlock:825 » UncheckedExecution
InstanceApiLiveTest.testDeleteInstance:223->BaseGoogleComputeEngineApiLiveTest.assertZoneOperationDoneSuccessfully:106 » NullPointer
GoogleComputeEngineServiceLiveTest>BaseComputeServiceLiveTest.testCreateAndRunAService:706->BaseComputeServiceLiveTest.createAndRunAServiceInGroup:723 » RunNodes
GoogleComputeEngineServiceLiveTest>BaseComputeServiceLiveTest.testAScriptExecutionAfterBootWithBasicTemplate:223 » RunNodes
GoogleComputeEngineServiceLiveTest>BaseComputeServiceLiveTest.testConcurrentUseOfComputeServiceToCreateNodes:475 » Execution
Tests run: 118, Failures: 5, Errors: 0, Skipped: 19
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61724899
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
Will take a stab at removing any redundant methods. Thanks for the review!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61707155
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
nope. listAtMarker is not redundant. it is used to implement the other 2!!
I don't think only supporting magic Iterator is a good choice anyway.
We have other places to focus on for now, imho.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61722828
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #223](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/223/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61717754
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Ignasi Barrera <no...@github.com>.
Nice! LGTM.
This should supersede my pagination pull request. The only thing left will be to remove from all APIs the `listFIrstPage` and `listAtMarker` methods. They are completely redundant, and the `ListOptions` already has the marker, so removing them and adapting the remaining code should be pretty straightforward.
Wanna add this to the PR? Otherwise I'll happily remove those methods in a separate PR once this one is in.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61704035
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Ignasi Barrera <no...@github.com>.
Ok, I see that.
Should then the `marker` method be removed feom the `ListOptions` object? I see it confusing and can conflict with the other marker parameter. If people want to list pages at a given marker, use the method designed for that purpose. Otherwise use the other ones, but don't allow to provide a "marker" option in the wrong method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61724294
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
running live tests
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61699649
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1623](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1623/) FAILURE
Looks like there's a problem with this pull request
[(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/78#issuecomment-61698161
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
I think the 3 lists ops are a fair balance. the magic one indirectly tests
the non-magic one. only real redundant is the magic without the options
param. I think that is worth keeping as it is easier to use, and better
than what we used to do (varargs which wreaks havoc on non-java). In other
words, let's give this a try!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61729155
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Ignasi Barrera <no...@github.com>.
APIs still have a redundant `listAtMarker` method. I thought about removing that one too and keep only the `list()` and the `list(options)` methods. Then the transform functions can call the latter by building the appropriate options object (which already has a field for the marker) and get rid of that reduntant `listAtMarker`. That is what I had in my PR. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61721041
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #218](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/218/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61697632
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
@nacx I'll merge this then move on to making things prettier :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61725639
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1627](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1627/) FAILURE
Looks like there's a problem with this pull request
[(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/78#issuecomment-61717190
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #221](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/221/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61699917
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1628](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1628/) 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/78#issuecomment-61718299
Re: [jclouds-labs-google] Migrate off PaginatedIterable to Iterator.
(#78)
Posted by Adrian Cole <no...@github.com>.
ok will move onto what you mentioned nextish.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/78#issuecomment-61726052