Skip to content

Conversation

@andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Jan 31, 2019

Tests #3735.

Blocked on

  • the fix in gax-java
  • a fix for another bug in gapic-generator to be released to artman
  • releasing gax-java
  • bumping the gax-java version in google-cloud-java

@andreamlin andreamlin requested a review from a team as a code owner January 31, 2019 01:14
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2019
@andreamlin andreamlin removed the request for review from a team January 31, 2019 01:14
@sduskis sduskis added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 7, 2019
@sduskis
Copy link
Contributor

sduskis commented Feb 7, 2019

@andreamlin, it looks like there are a bunch of merge conflicts.

@andreamlin andreamlin requested a review from sduskis February 7, 2019 18:15
@andreamlin
Copy link
Contributor Author

PTAL

fixed merge conflicts! test passes locally; hopefully the same can be said on kokoro

@andreamlin andreamlin removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 7, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019
} catch (Exception e) {
if (isNotExceptionType(exceptionTypes, e)) throw e;
}
Thread.sleep(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you add another sleep?

throws Exception {
try {
instanceClient.deleteInstance(INSTANCE_NAME);
Thread.sleep(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thread.sleep(1000) isn't great. Perhaps you can use Operation actualResponse = instanceClient.deleteInstance(INSTANCE_NAME); and poll?

.build();
Operation op3 = instanceClient.attachDiskInstance(INSTANCE_NAME, false, attachedDisk);
System.out.println(String.format("Disk attached: %s", op3.toString()));
Thread.sleep(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

sleeping again?

Iterable<SubnetworksScopedList> subnetworks =
subNetworkClient.aggregatedListSubnetworks(DEFAULT_PROJECT).iterateAll();
// TODO(andrealin): Implement futures for Compute methods.
Thread.sleep(10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the sleep? 10 seconds is a lot of time to for a test.

public class ITComputeTest {

private static final String ZONE = "us-central1-a";
private static final String ZONE = "us-west1-a";
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move REGION near here?


InstanceSettings instanceSettings =
InstanceSettings.newBuilder()
.setCredentialsProvider(FixedCredentialsProvider.create(credentials))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reuse the FixedCredentialsProvider for all three calls?

@Test
public void testDetachDisk() throws Exception {
long startAt = System.currentTimeMillis();
System.out.println("Clients created in " + (System.currentTimeMillis() - startAt) + "ms.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this print statement still belong here?

.addDisks(bootDisk)
.build();
Operation op1 = instanceClient.insertInstance(PROJECT_ZONE_NAME, instanceResource);
System.out.println(String.format("Instance created: %s", op1.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still belong here? If so, then perhaps a log statement would be better.

.setType(diskTypeName.toString())
.build();
Operation op2 = diskClient.insertDisk(PROJECT_ZONE_NAME, disk);
System.out.println(String.format("Disk created: %s", op2.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Log, and sleep?

Operation op3 = instanceClient.attachDiskInstance(INSTANCE_NAME, false, attachedDisk);
System.out.println(String.format("Disk attached: %s", op3.toString()));
Thread.sleep(10000);
System.out.println("----------------------------------------");
Copy link
Contributor

Choose a reason for hiding this comment

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

log statement?

System.out.println("----------------------------------------");

Operation op4 = instanceClient.detachDiskInstance(INSTANCE_NAME, DISK_NAME.getDisk());
System.out.println(String.format("Disk detached: %s", op4.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

log statement.

@sduskis sduskis added status: blocked Resolving the issue is dependent on other work. and removed 🚨 This issue needs some love. labels Feb 11, 2019
@andreamlin
Copy link
Contributor Author

Blocked on #4501 (LRO for Compute)

@sduskis sduskis added the api: compute Issues related to the Compute Engine API. label Mar 19, 2019
@sduskis
Copy link
Contributor

sduskis commented Apr 30, 2019

@andreamlin, it looks like this PR has gone stale. Please reopen once the blockers are removed.

@sduskis sduskis closed this Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: compute Issues related to the Compute Engine API. cla: yes This human has signed the Contributor License Agreement. status: blocked Resolving the issue is dependent on other work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants