feat(compute): extend compute hyperdisk pool create #9595

TetyanaYahodska · 2024-10-19T19:13:48Z

Description

Sample in NodeJs - GoogleCloudPlatform/nodejs-docs-samples#3826
Fixes #

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

Checklist

compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

minherz · 2024-10-24T17:28:20Z

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

    Assert.assertTrue(storagePool.getCapacityProvisioningType().equalsIgnoreCase("advanced"));
+    Assert.assertTrue(storagePool.getPerformanceProvisioningType().equalsIgnoreCase("advanced"));


please use PERFORMANCE_PROVISIONING_TYPE instead of "advanced" literal

Thank you. Done

compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java

minherz · 2024-10-29T16:00:58Z

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

    StoragePool storagePool = CreateHyperdiskStoragePool
         .createHyperdiskStoragePool(PROJECT_ID, ZONE, STORAGE_POOL_NAME, poolType,
-         "advanced", 10240, 10000, 10240);
+         "advanced", 10240, 10000, 10240,


should the capacity to be 1024 and not 10240?

It's not a scope of this task. I've created a new task to fix code and will follow yours recommendations.

I do not understand what you mean by "creating a new task"? Do you propose to address code review comments made in this PR by submitting a separate PR?
If you do, I find this proposal very inconvenient and time consuming. Please, explain what do you mean.

Regarding my question about the value 10240, I looked into Cloud Console UI and it limits the range to be from 1 to 1024 TB. I do not know if client library limits the range but I think that making it matching UX in Console is preferred.

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

minherz · 2024-10-29T16:03:25Z

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java


-  @Disabled
  @Test
  public void stage1_CreateHyperdiskStoragePoolTest()


why the name of the method follows snake notation and not pascal notation?

It's not a scope of this task. I've created a new task to fix code and will follow yours recommendations.

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

minherz · 2024-10-29T16:09:11Z

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

  public void stage2_CreateHyperdiskStoragePoolTest()
      throws IOException, ExecutionException, InterruptedException, TimeoutException {


what is being testing in this method that was not tested in the previous method? Why changing the arguments to the call to CreateDiskInStoragePool.createDiskInStoragePool() should matter for testing this code sample?
The code sample method does not have any conditions, so testing Google API invocation with different parameters does not test code sample but the backend API which is unnecessary.

It's not a scope of this task. I've created a new task to fix code and will follow yours recommendations.

I've fixed naming for these tests. As I understand from code and tests

the first testCreateHyperdiskStoragePool() tests that StoragePoll was created as expected

the second testCreateDiskInStoragePool() takes the StoragePoolLink of the StoragePool created in the first test and tests it's parameters.

according to your explanations the second test does not validate the code sample. the code sample shows how to create a storage pool. the test(s) should validate the code.

please, modify the test class to do one of the following, either to remove the second test because it does not add to the validation to code sample or perform all validations / asserts in the first test method.

We have two classes - CreateDiskInStoragePool and CreateHyperdiskStoragePool. To test this code Svyatoslav created two test methods - testCreateDiskInStoragePool and testCreateHyperdiskStoragePool. As far as I can see, these tests are working properly

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

minherz · 2024-10-30T18:25:09Z

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

    StoragePool storagePool = CreateHyperdiskStoragePool
         .createHyperdiskStoragePool(PROJECT_ID, ZONE, STORAGE_POOL_NAME, poolType,
-         "advanced", 10240, 10000, 10240);
+         "advanced", 10240, 10000, 10240,


I do not understand what you mean by "creating a new task"? Do you propose to address code review comments made in this PR by submitting a separate PR?
If you do, I find this proposal very inconvenient and time consuming. Please, explain what do you mean.

Regarding my question about the value 10240, I looked into Cloud Console UI and it limits the range to be from 1 to 1024 TB. I do not know if client library limits the range but I think that making it matching UX in Console is preferred.

minherz · 2024-10-30T18:32:54Z

@TetyanaYahodska I am puzzled by the multiple responses saying "It's not a scope of this task. I've created a new task ...".

The review is done for the code changed in this PR. However, the changes may influence the code that was not touched as well. In majority of situations splitting the work on the code review comments into multiple PRs makes the process to review and track changes harder than necessary.

If you find the feedback unclear or you disagree with the recommendations, please argument your point of view either in the comments or by scheduling a meeting.
If your opinion that some of the requested changes are hard to implement in this PR, please provide a clear explanation why it is hard. Once we agree about it, these changes should be captured as a new issue that is related to this PR. And then they can be handled as a separate task.

TetyanaYahodska · 2024-11-01T08:49:23Z

@minherz I don't mind making the changes you've asked me to make to code unrelated to this task. But as I know, the goal of each task should correspond to the implemented code and have an explanation for the changes made to the task. But if you want to review the big PRs, I'm open to your suggestions. Thank you for your patience!

minherz

Thank you for addressing all the comments. Please apply one remaining change to improve the code readability.

compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

minherz

lgtm. approving to avoid blocking your progress. however, before merging, please look to address the following:

apply changes to the test code described in this comment thread
i still do not get full understanding why the code uses 10240 instead of 1024 when passing the capacity value. I think it can be confusing to developers. Maybe the value is already 1024 and I am confused by Github UI. If it is still 10240, please have a look into the client library to see whether or not it should be 1024.

Thank you

minherz · 2024-11-06T19:35:25Z

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java

  public void stage2_CreateHyperdiskStoragePoolTest()
      throws IOException, ExecutionException, InterruptedException, TimeoutException {


according to your explanations the second test does not validate the code sample. the code sample shows how to create a storage pool. the test(s) should validate the code.

please, modify the test class to do one of the following, either to remove the second test because it does not add to the validation to code sample or perform all validations / asserts in the first test method.

TetyanaYahodska · 2024-11-07T09:41:59Z

@minherz After changing capacity to 1024 I've got error "message": "Provisioned capacity 1024 is smaller than the minimum allowed capacity 10240.",

TetyanaYahodska added 2 commits October 5, 2024 22:42

Added performance-provisioning-type, fixed test

371273a

merged changes from main

15c24c0

product-auto-label bot added api: compute Issues related to the Compute Engine API. samples Issues that are directly related to samples. labels Oct 19, 2024

TetyanaYahodska added 3 commits October 19, 2024 21:17

fixed test

d810b7e

Enabled tests

cef06ab

Fixed whitespaces

fbb8fa6

TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 19, 2024

TetyanaYahodska requested review from Sita04 and m-strzelczyk October 19, 2024 21:11

TetyanaYahodska assigned Sita04 Oct 19, 2024

TetyanaYahodska marked this pull request as ready for review October 19, 2024 21:12

TetyanaYahodska requested review from a team and yoshi-approver as code owners October 19, 2024 21:12

minherz requested changes Oct 24, 2024

View reviewed changes

minherz assigned minherz and unassigned Sita04 Oct 25, 2024

Merge branch 'main' into extend-compute_hyperdisk_pool_create

81891b3

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 28, 2024

Added comments

55c7835

TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 28, 2024

minherz requested changes Oct 29, 2024

View reviewed changes

Merge branch 'main' into extend-compute_hyperdisk_pool_create

47ecd63

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 30, 2024

TetyanaYahodska added 2 commits October 30, 2024 15:30

Fixed comments and zone

8294ad9

Fixed timeout and zone in test class

928be83

TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 30, 2024

minherz requested changes Oct 30, 2024

View reviewed changes

Merge branch 'main' into extend-compute_hyperdisk_pool_create

fdbb0f2

kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Oct 30, 2024

Fixed naming, added variable CAPACITY_PROVISIONING_TYPE

df550e2

minherz requested changes Nov 1, 2024

View reviewed changes

compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java Outdated Show resolved Hide resolved

compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java Show resolved Hide resolved

TetyanaYahodska added 2 commits November 4, 2024 18:37

Fixed storagePoolType

3efab67

Fixed lint issue

9fd467d

TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 4, 2024

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 4, 2024

TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 4, 2024

minherz approved these changes Nov 6, 2024

View reviewed changes

Merge branch 'main' into extend-compute_hyperdisk_pool_create

6dbb870

kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Nov 7, 2024

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 7, 2024

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 7, 2024

TetyanaYahodska added the kokoro:run Add this label to force Kokoro to re-run the tests. label Nov 7, 2024

TetyanaYahodska merged commit 5c97f27 into main Nov 7, 2024
8 checks passed

TetyanaYahodska deleted the extend-compute_hyperdisk_pool_create branch November 7, 2024 10:41

		Assert.assertTrue(storagePool.getCapacityProvisioningType().equalsIgnoreCase("advanced"));
		Assert.assertTrue(storagePool.getPerformanceProvisioningType().equalsIgnoreCase("advanced"));

		public void stage2_CreateHyperdiskStoragePoolTest()
		throws IOException, ExecutionException, InterruptedException, TimeoutException {

feat(compute): extend compute hyperdisk pool create #9595

feat(compute): extend compute hyperdisk pool create #9595

Uh oh!

Conversation

TetyanaYahodska commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minherz commented Oct 30, 2024

Uh oh!

TetyanaYahodska commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minherz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

minherz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TetyanaYahodska commented Nov 7, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TetyanaYahodska commented Oct 19, 2024 •

edited

Loading

TetyanaYahodska commented Nov 1, 2024 •

edited

Loading