[BEAM-1988] Migrate from utils.path to BFS#2643
Conversation
|
Retest this please |
| self.google_cloud_options.staging_location = filesystem.join( | ||
| self.google_cloud_options.staging_location, path_suffix) | ||
| self.google_cloud_options.temp_location = utils.path.join( | ||
| self.google_cloud_options.temp_location = filesystem.join( |
There was a problem hiding this comment.
What happens if temp_location is not a gcs location? It is an error by itself but the join part was handled correctly in the previous code.
I wonder if there is any value in something like bfs.join where bfs would call the right join method for the given type, avoiding calls to get_file_system.
There was a problem hiding this comment.
Yes, I think me and Cham discussed it at some point. I can work on that in another PR and clean the get_filesystem infiltration everywhere.
| @@ -284,13 +285,15 @@ def stage_job_resources( | |||
| raise RuntimeError( | |||
| 'The --temp_location option must be specified.') | |||
There was a problem hiding this comment.
Why do we check for temp_location here?
There was a problem hiding this comment.
Thank you does not need to happen in this PR if it is involved.
There was a problem hiding this comment.
This is just a sanity check as https://github.com/apache/beam/blob/master/sdks/python/apache_beam/runners/dataflow/internal/apiclient.py#L325 would have already assigned temp_location to staging_location had it not been specified. I think we can leave it as is right now.
|
This PR LGTM as is. Are you planning to make any more changes in this PR? |
|
No the two changes we discussed seem unrelated so I can do them in separate PRs. |
3 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.
R: @chamikaramj @aaltay PTAL