Skip to content

feat(compute): add compute consistency group clone #9693

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.

Sign up for GitHub

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

Jump to bottom
Closed
TetyanaYahodska wants to merge 31 commits into from
Closed

feat(compute): add compute consistency group clone #9693

TetyanaYahodska wants to merge 31 commits into from

Conversation

@TetyanaYahodska
Copy link
Contributor

@TetyanaYahodska TetyanaYahodska commented Nov 22, 2024 •
edited
Loading

Description

Documentation - https://cloud.google.com/compute/docs/disks/async-pd/manage-async-disks?authuser=0#clone_all_disks_in_a_consistency_group
Node.js sample - GoogleCloudPlatform/nodejs-docs-samples#3920

Fixes #

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed Sample Format Guide
  • pom.xml parent set to latest shared-configuration
  • Appropriate changes to README are included in PR
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
  • These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • Tests pass: mvn clean verify required
  • Lint passes: mvn -P lint checkstyle:check required
  • Static Analysis: mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

@product-auto-label product-auto-label bot added api: compute Issues related to the Compute Engine API. samples Issues that are directly related to samples. labels Nov 22, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 22, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 22, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 22, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 22, 2024
@TetyanaYahodska TetyanaYahodska added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 22, 2024
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 23, 2024
@TetyanaYahodska TetyanaYahodska added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 23, 2024
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 23, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 24, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 24, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 24, 2024
@TetyanaYahodska TetyanaYahodska marked this pull request as ready for review November 24, 2024 10:02
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 1, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 1, 2024
minherz
minherz requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

we prefer simple code samples to complex one. this one uses heuristic that is not easy to comprehend to distinguish between regional and zonal disks. There is also a potentially confusing use of multiple region variables despite the fact that the location for the group and the disk should be the same (in case of the region).

...loud-client/src/main/java/compute/disks/consistencygroup/CloneDisksFromConsistencyGroup.java Outdated
Comment on lines 57 to 85
if (Character.isDigit(disksLocation.charAt(disksLocation.length() - 1))) {
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests.
try (RegionDisksClient disksClient = RegionDisksClient.create()) {
BulkInsertRegionDiskRequest request = BulkInsertRegionDiskRequest.newBuilder()
.setProject(project)
.setRegion(disksLocation)
.setBulkInsertDiskResourceResource(
BulkInsertDiskResource.newBuilder()
.setSourceConsistencyGroupPolicy(sourceConsistencyGroupPolicy)
.build())
.build();

response = disksClient.bulkInsertAsync(request).get(3, TimeUnit.MINUTES);
}
} else {
try (DisksClient disksClient = DisksClient.create()) {
BulkInsertDiskRequest request = BulkInsertDiskRequest.newBuilder()
.setProject(project)
.setZone(disksLocation)
.setBulkInsertDiskResourceResource(
BulkInsertDiskResource.newBuilder()
.setSourceConsistencyGroupPolicy(sourceConsistencyGroupPolicy)
.build())
.build();

response = disksClient.bulkInsertAsync(request).get(3, TimeUnit.MINUTES);
}
}
Copy link
Contributor

@minherz minherz Dec 9, 2024

Choose a reason for hiding this comment

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

this code does two code samples in one. please, fix this either by demonstrating cloning on a zonal disk or creating two code samples for different demos. I recommend to reach out to tech writer to see if it makes sense to have two samples.
Note that the current documentation use of pseudo "LOCATION" parameter that is further have to be edited by a user is not a recommended practice.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Dec 10, 2024

Choose a reason for hiding this comment

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

Thank you! Created sample for zonal location

...loud-client/src/main/java/compute/disks/consistencygroup/CloneDisksFromConsistencyGroup.java Outdated
Comment on lines 39 to 43
String disksLocation = "us-central1";
// Name of the consistency group you want to clone disks from.
String consistencyGroupName = "YOUR_CONSISTENCY_GROUP_NAME";
// Name of the region in which your consistency group is located.
String consistencyGroupLocation = "us-central1";
Copy link
Contributor

@minherz minherz Dec 9, 2024

Choose a reason for hiding this comment

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

Note that consistency group cannot include disks from a region other than the group's region. I suggest to reflect it in the code by using a single parameter for disk location (for regional disks). Otherwise, use zone disks in the same zone explicitly.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Dec 10, 2024

Choose a reason for hiding this comment

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

Thank you! Fixed

@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 10, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 13, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 13, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 13, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@TetyanaYahodska TetyanaYahodska added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Dec 18, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@TetyanaYahodska TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 18, 2024
@minherz minherz added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 19, 2024
@minherz
Copy link
Contributor

minherz commented Dec 19, 2024 •
edited
Loading

This PR cannot be merged. Merging the main branch into PR's branch is forbidden pattern. It introduces changes that cannot be effectively tracked in the commit history and increase the complexity of the review. When PR's branch gets out of sync with the main it has to be rebased to the main branch and not merged.

I recommend to close this PR and submit the original changes that address all review feedback received so far as a new PR. Alternative to it is to undo all changes caused by the merge.

I converted PR to draft pending your decision.

@minherz minherz marked this pull request as draft December 19, 2024 04:30
@TetyanaYahodska TetyanaYahodska deleted the compute_consistency_group_clone branch December 19, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@minherz minherz minherz requested changes

@yoshi-approver yoshi-approver Awaiting requested review from yoshi-approver yoshi-approver is a code owner

Assignees

@minherz minherz

Labels

api: compute Issues related to the Compute Engine API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. kokoro:run Add this label to force Kokoro to re-run the tests. samples Issues that are directly related to samples.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TetyanaYahodska @minherz @kokoro-team