Skip to content

ARROW-14912: [C++] implement GcsFileSystem::CopyFile - #11797

Closed
coryan wants to merge 2 commits into
apache:masterfrom
coryan:ARROW-14912-implement-gcs-file-system-copy-file
Closed

ARROW-14912: [C++] implement GcsFileSystem::CopyFile#11797
coryan wants to merge 2 commits into
apache:masterfrom
coryan:ARROW-14912-implement-gcs-file-system-copy-file

Conversation

@coryan

@coryancoryan commented Nov 30, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actionsBot commented Nov 30, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14912

@coryan

coryan commented Nov 30, 2021

Copy link
Copy Markdown
ContributorAuthor

The Integration / AMD64 Conda Integration Test failure seems unrelated and may be a transient failure:

Error: Failed to execute goal on project arrow-java-root: Could not resolve dependencies for project org.apache.arrow:arrow-java-root:pom:7.0.0-SNAPSHOT: The following artifacts could not be resolved: org.slf4j:log4j-over-slf4j:jar:1.7.25, org.junit.platform:junit-platform-runner:jar:1.4.0: Could not transfer artifact org.slf4j:log4j-over-slf4j:jar:1.7.25 from/to central (https://repo.maven.apache.org/maven2): Connection reset -> [Help 1]

@coryan
coryan marked this pull request as ready for review November 30, 2021 02:10
pitrou
pitrou reviewed Nov 30, 2021
View reviewed changes

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for doing this. LGTM, just a suggestion.

Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc Outdated
auto fs = internal::MakeGcsFileSystemForTest(TestGcsOptions());
const auto destination_path = kPreexistingBucket + std::string("/copy-destination");
const auto status = fs->CopyFile(NotFoundObjectPath(), destination_path);
EXPECT_EQ(status.code(), StatusCode::IOError);

@pitroupitrouNov 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this can be shortened to:

ASSERT_RAISES(IOError, fs->CopyFile(NotFoundObjectPath(), destination_path));

If the error message is predictable enough, you could also test for it using e.g.:

EXPECT_RAISES_WITH_MESSAGE_THAT(IOError, ::testing::HasSubstr("xxx"),
fs->CopyFile(NotFoundObjectPath(), destination_path));

@coryancoryanNov 30, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done. PTAL.

@ursabot

ursabot commented Nov 30, 2021•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 8daa7a4 and contender = 996a08b. 996a08b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.45% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

@coryan
coryan deleted the ARROW-14912-implement-gcs-file-system-copy-file branch November 30, 2021 14:31
@asfimportasfimport mentioned this pull request Dec 1, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@coryan@ursabot@pitrou