[BEAM-8472] Get default GCP region from gcloud (Java)#9974
[BEAM-8472] Get default GCP region from gcloud (Java)#9974ibzib merged 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
- Is 1 second enough?
- Should we check the return value of this call? (IIUC it returns false for timeout.)
There was a problem hiding this comment.
Is 1 second enough?
Tests on my machine (in debug mode) indicated it took on average around 0.25 seconds, including reading the output stream after. I have bumped the timeout to 2s just to make double sure.
Should we check the return value of this call?
Not sure it matters, since I think it should interrupt the process after the time limit has passed, but it couldn't hurt to check anyway.
ibzib
force-pushed
the
java-default-region
branch
2 times, most recently
from
5102f46 to
c46c2e8
Compare
|
Run Java PreCommit |
There was a problem hiding this comment.
Is there a reason why we aren't using the default instance factory to configure the region instead of detecting and setting it here. This would mean that all users of the pipeline option would get it and not just Dataflow.
See DefaultProjectFactory for an example.
ibzib
force-pushed
the
java-default-region
branch
from
c46c2e8 to
8b005ed
Compare
ibzib
force-pushed
the
java-default-region
branch
from
8b005ed to
8e49c5a
Compare
| @Test | ||
| public void testDefaultGcpRegion() { | ||
| DataflowPipelineOptions options = PipelineOptionsFactory.as(DataflowPipelineOptions.class); | ||
| assertEquals("us-central1", options.getRegion()); |
There was a problem hiding this comment.
This test will fail on any machine where the CLOUDSDK_COMPUTE_REGION is defined or gcloud config compute/region is set.
You can test your code by instantiating the DefaultGcpRegionFactory directly and ensuring that it is factored in such a way where you can pass in the environment map directly to it and similarly for testing the process execution.
There was a problem hiding this comment.
Thanks for catching this Luke. Filed #10048 to improve this.
3 participants
Same as #9868 but more verbose 😄
As with the Python version, I've verified this works locally. This doesn't fit into a unit test because it requires special environment configuration and/or calling an external process, but nor does it seem to warrant an integration test.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.