Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Compute): Use paginated lists. #1445

Merged
merged 2 commits into from
Aug 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions compute/api/Compute.Samples.Tests/ComputeFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@ public class ComputeFixture : IDisposable, ICollectionFixture<ComputeFixture>

public string DiskImage => "projects/debian-cloud/global/images/family/debian-10";

public string DiskSizeGb => "10";
public long DiskSizeGb => 10;

public string NetworkName => "default";

public string UsageReportBucketName { get; } = GenerateName("b");

public string UsageReportPrefix { get; } = "test-usage";
public string UsageReportPrefix => "test-usage";

public string PublicImagesProjectId => "windows-sql-cloud";

private IList<string> MachinesToDelete { get; } = new List<string>();

Expand Down
34 changes: 34 additions & 0 deletions compute/api/Compute.Samples.Tests/ListImagesAsyncTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

using System.Threading.Tasks;
using Xunit;

namespace Compute.Samples.Tests
{
[Collection(nameof(ComputeFixture))]
public class ListImagesAsyncTest
{
private readonly ComputeFixture _fixture;
private readonly ListImagesAsyncSample _listSample = new ListImagesAsyncSample();

public ListImagesAsyncTest(ComputeFixture fixture) => _fixture = fixture;

[Fact]
public async Task ListsImages() => // Just test that it doesn't fail.
await _listSample.ListImagesAsync(_fixture.PublicImagesProjectId);
}
}
35 changes: 35 additions & 0 deletions compute/api/Compute.Samples.Tests/ListImagesPerPageAsyncTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

using System.Threading.Tasks;
using Xunit;

namespace Compute.Samples.Tests
{
[Collection(nameof(ComputeFixture))]
public class ListImagesPerPageAsyncTest
{
private readonly ComputeFixture _fixture;
private readonly ListImagesPerPageAsyncSample _listSample = new ListImagesPerPageAsyncSample();

public ListImagesPerPageAsyncTest(ComputeFixture fixture) => _fixture = fixture;

[Fact]
public async Task ListsImagesPerPage() => // Just test that it doesn't fail.
await _listSample.ListImagesPerPageAsync(_fixture.PublicImagesProjectId);

}
}
2 changes: 1 addition & 1 deletion compute/api/Compute.Samples/Compute.Samples.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Google.Cloud.Compute.V1" Version="1.0.0-alpha02" />
<PackageReference Include="Google.Cloud.Compute.V1" Version="1.0.0-beta01" />
</ItemGroup>

</Project>
2 changes: 1 addition & 1 deletion compute/api/Compute.Samples/CreateInstanceAsync.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public async Task CreateInstanceAsync(
string machineName = "test-machine",
string machineType = "n1-standard-1",
string diskImage = "projects/debian-cloud/global/images/family/debian-10",
string diskSizeGb = "10",
long diskSizeGb = 10,
string networkName = "default")
{
Instance instance = new Instance
Expand Down
23 changes: 6 additions & 17 deletions compute/api/Compute.Samples/ListAllInstancesAsync.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,31 +30,20 @@ public async Task<IList<Instance>> ListAllInstancesAsync(
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests.
InstancesClient client = await InstancesClient.CreateAsync();
InstanceAggregatedList instanceList;
IList<Instance> allInstances = new List<Instance>();

// Make the request to list all VM instances in a project.
AggregatedListInstancesRequest request = new AggregatedListInstancesRequest { Project = projectId };
do
await foreach (var instancesByZone in client.AggregatedListAsync(projectId))
{
instanceList = await client.AggregatedListAsync(request);
// The result contains a KeyValuePair collection, where the key is a zone and the value
// is a collection of instances in that zone.
foreach (var instancesByZone in instanceList.Items)
Console.WriteLine($"Instances for zone: {instancesByZone.Key}");
foreach (var instance in instancesByZone.Value.Instances)
{
Console.WriteLine($"Instances for zone: {instancesByZone.Key}");
foreach (var instance in instancesByZone.Value.Instances)
{
Console.WriteLine($"-- Name: {instance.Name}");
allInstances.Add(instance);
}
Console.WriteLine($"-- Name: {instance.Name}");
allInstances.Add(instance);
}
// Use the NextPageToken value on the request result to make subsequent requests
// until all instances have been listed.
request.PageToken = instanceList.NextPageToken;

// When all instances are listed the last result NextPageToken is not set.
} while (instanceList.HasNextPageToken);
}

return allInstances;
}
Expand Down
54 changes: 54 additions & 0 deletions compute/api/Compute.Samples/ListImagesAsync.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// [START compute_images_list]

using Google.Cloud.Compute.V1;
using System;
using System.Threading.Tasks;

public class ListImagesAsyncSample
{
public async Task ListImagesAsync(
// TODO(developer): Set your own default values for these parameters or pass different values when calling this method.
string projectId = "your-project-id")
{
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests.
ImagesClient client = await ImagesClient.CreateAsync();

// Make the request to list all non-deprecated images in a project.
ListImagesRequest request = new ListImagesRequest
{
Project = projectId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MaxResults = 2,
Kindly add MaxResults parameter, which tells the number of images to be displayed in each page.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same response as below, I've set it to MaxResult = 100.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with 100 here, because we are not showing results page by page and just using this under the hood to generate multiple requests.

// Listing only non-deprecated images to reduce the size of the reply.
Filter = "deprecated.state != DEPRECATED",
// MaxResults indicates the maximum number of items that will be returned per page.
MaxResults = 100
};

// Although the MaxResults parameter is specified in the request, the sequence returned
// by the ListAsync() method hides the pagination mechanic. The library makes multiple
// requests to the API for you, so you can simply iterate over all the images.
await foreach (var image in client.ListAsync(request))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  // Although the `MaxResults` parameter is specified in the request, the iterable returned
  // by the `ListAsync()` method hides the pagination mechanic. The library makes multiple
  // requests to the API for you, so you can simply iterate over all the images.

Kindly add the above comment. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, with a slight word change, becuse it's more idiomatic: iterable => sequence

{
// The result is an Image collection.
Console.WriteLine($"Image: {image.Name}");
}
}
}

// [END compute_images_list]
60 changes: 60 additions & 0 deletions compute/api/Compute.Samples/ListImagesPerPageAsync.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// [START compute_images_list_page]

using Google.Cloud.Compute.V1;
using System;
using System.Threading.Tasks;

public class ListImagesPerPageAsyncSample
{
public async Task ListImagesPerPageAsync(
// TODO(developer): Set your own default values for these parameters or pass different values when calling this method.
string projectId = "your-project-id")
{
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests.
ImagesClient client = await ImagesClient.CreateAsync();

// Make the request to list all non-deprecated images in a project.
ListImagesRequest request = new ListImagesRequest
{
Project = projectId,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MaxResults = 2,
Kindly add MaxResults parameter, which tells the number of images to be displayed in each page.
If not added, all images will be displayed in a single page.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not added, all images will be displayed in a single page.

Not really, MaxResults has a default value of 500 as specified on API ref docs

That said, I'm completely fine with adding an explicit MaxResults for demonstration purposes but I don't think it should have a value of 2, this is a very low page size value, and will provoke plenty of requests to the server for anyone listing even a few images.
I'm not even worried about our sample tests, I'm worried about users copying this code as is and including it in their projects, and then generating all this unnecessary trafic for their own application.
I'll set to 100, and I'm happy to adjust it some more. Also, please, consider adjusting this value to something far greater than 2 for samples in other languages as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used 10 in PHP and Python for the paginated results as this seemed to be a good number for demonstration purposes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes 10 seems better, I did list sevaral of the public image sets and couldn't find any set containing more than 200 non-deprecated images. I'll change here to 10, but leave the fully automatic one on 100.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Amanda for the explanation! I understand the rational behind the param setting. Will double check the param values in other languages as well.

// Listing only non-deprecated images to reduce the size of the reply.
Filter = "deprecated.state != DEPRECATED",
// MaxResults indicates the maximum number of items that will be returned per page.
MaxResults = 10
};

// Call the AsRawResponses() method of the returned image sequence to access the page sequece instead
// This allows you to have more granular control of iteration over paginated results from the API.
// Each time you access the next page, the library retrieves that page from the API.
int pageIndex = 0;
await foreach (var page in client.ListAsync(request).AsRawResponses())
{
Console.WriteLine($"Page index: {pageIndex}");
pageIndex++;
foreach (var image in page)
{
// The result is an Image collection.
Console.WriteLine($"Image: {image.Name}");
}
}
}
}

// [END compute_images_list_page]
23 changes: 4 additions & 19 deletions compute/api/Compute.Samples/ListZoneInstancesAsync.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,30 +31,15 @@ public async Task<IList<Instance>> ListZoneInstancesAsync(
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests.
InstancesClient client = await InstancesClient.CreateAsync();
InstanceList instanceList;
IList<Instance> allInstances = new List<Instance>();

// Make the request to list all VM instances in the given zone in the specified project.
ListInstancesRequest request = new ListInstancesRequest
{
Project = projectId,
Zone = zone,
};
do
await foreach(var instance in client.ListAsync(projectId, zone))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to await foreach here would be to await client.ListAsync(projectId, zone).ToListAsync() to come up with allInstances, then synchronously iterate over that, but I'm not bothered either way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one gives you he chance to break in the middle of the async iteration without having fetched everything, right? Even if we are not showing that,...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. (And will show results as it goes, of course.) I just twitch at manually adding to the list ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that's for the purposes of the tests only, but I do see what you mean about that making you twitch. We have several tests that do this kind of thing across the repo. It's the alternative to testing against stdout.

{
instanceList = await client.ListAsync(request);
// The result is an Instance collection.
foreach (var instance in instanceList.Items)
{
Console.WriteLine($"-- Name: {instance.Name}");
allInstances.Add(instance);
}
// Use the NextPageToken value on the request result to make subsequent requests
// until all instances have been listed.
request.PageToken = instanceList.NextPageToken;

// When all instances are listed the last result NextPageToken is not set.
} while (instanceList.HasNextPageToken);
Console.WriteLine($"Instance: {instance.Name}");
allInstances.Add(instance);
}

return allInstances;
}
Expand Down