Skip to content

[BEAM-6821] FileBasedSink improper paths #8054

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
Merged
chamikaramj merged 2 commits into from
Jun 7, 2019
Merged

[BEAM-6821] FileBasedSink improper paths #8054

chamikaramj merged 2 commits into from
Jun 7, 2019

Conversation

@gkovelman
Copy link
Contributor

@gkovelman gkovelman commented Mar 14, 2019 •
edited
Loading

File path generated in open_writer method is not according to target filesystem, because os.path.join is used and not FileSystems.join.
This created incompatibilities between, for example, Windows and GCS.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status		 Build Status Build Status Build Status
Python Build Status
Build Status		 --- Build Status
Build Status		 Build Status --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@gkovelman
[BEAM-6821] FileBasedSink improper paths
9ab6cfd 
File path generated in open_writer method is not according to target filesystem, because os.path.join is used and not FileSystems.join.
This created incompatibilities between, for example, Windows and GCS.
@gkovelman
Copy link
Contributor Author

gkovelman commented Mar 14, 2019

R: @tvalentyn

@gkovelman
Copy link
Contributor Author

gkovelman commented Mar 14, 2019

Run Python PostCommit

@tvalentyn
Copy link
Contributor

tvalentyn commented Mar 14, 2019

retest this please

@tvalentyn
Copy link
Contributor

tvalentyn commented Mar 14, 2019

Thanks, @gkovelman . I didn't look why the test failed for this PR, but we might need to wait for a rollback to be merged: #8059.

@gkovelman
Copy link
Contributor Author

gkovelman commented Mar 17, 2019

Run Python PreCommit

2 similar comments
@gkovelman
Copy link
Contributor Author

gkovelman commented Mar 19, 2019

Run Python PreCommit

@tvalentyn
Copy link
Contributor

tvalentyn commented Mar 26, 2019

Run Python PreCommit

@tvalentyn
Copy link
Contributor

tvalentyn commented Mar 26, 2019

Run Python PostCommit

tvalentyn
tvalentyn approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@tvalentyn tvalentyn left a comment

Choose a reason for hiding this comment

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

LGTM as long as tests pass.

@gkovelman
Copy link
Contributor Author

gkovelman commented Mar 27, 2019

Run Python PreCommit

1 similar comment
@gkovelman
Copy link
Contributor Author

gkovelman commented Mar 30, 2019

Run Python PreCommit

@tvalentyn
Copy link
Contributor

tvalentyn commented Apr 1, 2019

Please take a look at test failures: Click on Details -> Gradle Build Scan.

There is a lint error and a test failure.

To run an individual test locally see: https://cwiki.apache.org/confluence/display/BEAM/Contribution+Testing+Guide#ContributionTestingGuide-HowtorunPythonunittests

Perhaps that's a flake.

@gkovelman
Copy link
Contributor Author

gkovelman commented Apr 8, 2019

Run Python PreCommit

@gkovelman
Copy link
Contributor Author

gkovelman commented Apr 8, 2019

Run Python PostCommit

@gkovelman
Copy link
Contributor Author

gkovelman commented Apr 8, 2019

@tvalentyn It's fixed now

tvalentyn
tvalentyn approved these changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@tvalentyn tvalentyn left a comment

Choose a reason for hiding this comment

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

Thanks, @gkovelman.
Adding @chamikaramj who could merge.

@stale
Copy link

stale bot commented Jun 7, 2019

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Jun 7, 2019
@tvalentyn
Copy link
Contributor

tvalentyn commented Jun 7, 2019

Ping @chamikaramj

@stale stale bot removed the stale label Jun 7, 2019
@tvalentyn
Copy link
Contributor

tvalentyn commented Jun 7, 2019

Sorry, @gkovelman, looks like this PR fell off the radar - please ping the PR thread if you don't receive a response within couple of days.

@chamikaramj
Copy link
Contributor

chamikaramj commented Jun 7, 2019

LGTM. Thanks.

@chamikaramj
Copy link
Contributor

chamikaramj commented Jun 7, 2019

Retest this please

@chamikaramj
Copy link
Contributor

chamikaramj commented Jun 7, 2019

Run Python PostCommit

@chamikaramj chamikaramj merged commit 13fcf78 into apache:master Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tvalentyn tvalentyn tvalentyn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gkovelman @tvalentyn @chamikaramj