Skip to content

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

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

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

Conversation

@coryan
Copy link
Contributor

@coryan coryan commented Nov 30, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Nov 30, 2021

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

@coryan
Copy link
Contributor Author

coryan commented Nov 30, 2021

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
Copy link
Member

@pitrou pitrou left a comment

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.

cpp/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);
Copy link
Member

@pitrou pitrou Nov 30, 2021

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));

Copy link
Contributor Author

@coryan coryan Nov 30, 2021

Choose a reason for hiding this comment

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

Done. PTAL.

@pitrou pitrou closed this in 996a08b Nov 30, 2021
@ursabot
Copy link

ursabot commented Nov 30, 2021 •
edited
Loading

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
@asfimport asfimport mentioned this pull request Dec 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou 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