Skip to content

ARROW-14236: [C++] Add GCS testbench for testing - #11375

Closed
coryan wants to merge 10 commits into
apache:masterfrom
coryan:ARROW-14236-install-gcs-testbench
Closed

ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom
coryan:ARROW-14236-install-gcs-testbench

Conversation

@coryan

@coryancoryan commented Oct 9, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actionsBot commented Oct 9, 2021

Copy link
Copy Markdown

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

@coryan
coryan marked this pull request as draft October 9, 2021 19:16
@coryan
coryan marked this pull request as ready for review October 9, 2021 23:20
emkornfield
emkornfield reviewed Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc Outdated
emkornfield
emkornfield reviewed Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc Outdated
emkornfield
emkornfield reviewed Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should status be passed with const ref?

@coryancoryanOct 11, 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.

emkornfield
emkornfield reviewed Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc Outdated
@emkornfield

emkornfield commented Oct 10, 2021

Copy link
Copy Markdown
Contributor

Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou

emkornfield
emkornfield reviewed Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc Outdated
emkornfield
emkornfield reviewed Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt Outdated
kou
kou reviewed Oct 11, 2021
View reviewed changes

@koukou 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.

We already use boost::process for S3 tests. So it's not a new dependency.

Comment threadci/docker/debian-10-cpp.dockerfile Outdated
@coryan

coryan commented Oct 11, 2021

Copy link
Copy Markdown
ContributorAuthor

We already use boost::process for S3 tests. So it's not a new dependency.

Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.

@kou

kou commented Oct 11, 2021

Copy link
Copy Markdown
Member

We already use boost::process for S3 tests. So it's not a new dependency.

Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.

Sure. This is an answer of

#11375 (comment)

boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?

.

I wanted to say that we don't need to think about boost::process in this pull request.

@coryan

coryan commented Oct 11, 2021

Copy link
Copy Markdown
ContributorAuthor

I wanted to say that we don't need to think about boost::process in this pull request.

Thanks. I removed the change to vcpkg.json.

emkornfield
emkornfield reviewed Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc Outdated
@emkornfield

emkornfield commented Oct 11, 2021

Copy link
Copy Markdown
Contributor

C++ LGTM, @kou any more concerns?

kou
kou approved these changes Oct 12, 2021
View reviewed changes

@koukou 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.

I removed the change to vcpkg.json.

Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...

I think that we can merge this with minor Dockerfile related improvements.

Comment threadci/docker/debian-10-cpp.dockerfile Outdated
Comment threadci/docker/debian-11-cpp.dockerfile Outdated
Comment threadci/docker/fedora-33-cpp.dockerfile Outdated
Comment threadci/docker/linux-apt-r.dockerfile Outdated
Comment threadci/docker/ubuntu-20.04-cpp.dockerfile Outdated
Comment threadci/docker/ubuntu-20.10-cpp.dockerfile Outdated
Comment threadci/docker/ubuntu-21.04-cpp.dockerfile Outdated
@kou

kou commented Oct 12, 2021

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g nightly

@github-actions

github-actionsBot commented Oct 12, 2021

Copy link
Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8

Submitted crossbow builds: ursacomputing/crossbow @ actions-916

TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions

@kou

kou commented Oct 12, 2021

Copy link
Copy Markdown
Member

Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?

@pitrou

pitrou commented Oct 12, 2021

Copy link
Copy Markdown
Member

Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?

How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.

pitrou
pitrou reviewed Oct 12, 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 very much @coryan . Here are a number of minor comments.

Comment threadcpp/src/arrow/filesystem/gcsfs.cc Outdated

@pitroupitrouOct 12, 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.

You can simply write:

ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));

@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.

Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).

@coryancoryanOct 12, 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.

Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?

@coryancoryanOct 12, 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.

Fixed.

Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:

AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);

@coryancoryanOct 12, 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.

@pitrou

pitrou commented Oct 12, 2021

Copy link
Copy Markdown
Member

The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:

#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()

@coryan

coryan commented Oct 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?

Which ones? Some of them already have these settings, e.g.:

https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266

How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.

That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).

@pitrou

pitrou commented Oct 12, 2021

Copy link
Copy Markdown
Member

The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:

I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.

@kou

kou commented Oct 12, 2021

Copy link
Copy Markdown
Member

Some of them already have these settings

Oh, sorry. I missed them.

@coryan

coryan commented Oct 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I think I addressed all the comments, please let me know if I missed something.

@pitrou

pitrou commented Oct 13, 2021

Copy link
Copy Markdown
Member

Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:

[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou approved these changes Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)

@coryan

coryan commented Oct 13, 2021

Copy link
Copy Markdown
ContributorAuthor

Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:

[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:

[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:

54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.

@coryan

coryan commented Oct 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I will open a separate bug.

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

@pitrou

pitrou commented Oct 13, 2021

Copy link
Copy Markdown
Member

Thank you, I'll merge this one then.

@pitrou

pitrou commented Oct 13, 2021

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g cpp

@github-actions

github-actionsBot commented Oct 13, 2021

Copy link
Copy Markdown

Revision: f3edc48

Submitted crossbow builds: ursacomputing/crossbow @ actions-935

TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions

@pitroupitrou changed the title ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot

ursabot commented Oct 13, 2021•
edited
Loading

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench branch October 13, 2021 18:14
@asfimportasfimport mentioned this pull request Oct 14, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
ARROW-14236: [C++] Add GCS testbench for testing by coryan · Pull Request #11375 · apache/arrow · GitHub
Skip to content
ARROW-14236: [C++] Add GCS testbench for testing - #11375
Closed
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Closed
ARROW-14236: [C++] Add GCS testbench for testing#11375
coryan wants to merge 10 commits into
apache:masterfrom 
coryan:ARROW-14236-install-gcs-testbench
Conversation
@coryan
@coryancoryan
commented
Oct 9, 2021

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Oct 9, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14236
@coryan
coryan
marked this pull request as draft
October 9, 2021 19:16
@coryan
coryan
marked this pull request as ready for review
October 9, 2021 23:20
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfieldemkornfieldOct 10, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

should status be passed with const ref?
@coryancoryanOct 11, 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.
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@emkornfield
emkornfield
commented
Oct 10, 2021

Copy link


Copy Markdown

Contributor
Left a few comments. boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)? CC @pitrou@kou
emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

emkornfield
emkornfield
reviewed
Oct 10, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/CMakeLists.txt
Outdated

kou
kou
reviewed
Oct 11, 2021
View reviewed changes
@koukou
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.

We already use boost::process for S3 tests. So it's not a new dependency.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
@kou
kou
commented
Oct 11, 2021

Copy link


Copy Markdown

Member
We already use boost::process for S3 tests. So it's not a new dependency.
Could you clarify what part of the change are you commenting on? Is it the changes to vcpkg.json? If so, while it is true that boost::process was used already, it was not declared as a vcpkg dependency and thus not built by default.
Sure. This is an answer of
#11375 (comment)
boost process seems like a new dependency on boost? I don't know if there are any issue taking it (I guess this is only for tests)?
.
I wanted to say that we don't need to think about boost::process in this pull request.
@coryan
coryan
commented
Oct 11, 2021

Copy link


Copy Markdown

ContributorAuthor
I wanted to say that we don't need to think about boost::process in this pull request.
Thanks. I removed the change to vcpkg.json.
emkornfield
emkornfield
reviewed
Oct 11, 2021
View reviewed changes
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@emkornfield
emkornfield
commented
Oct 11, 2021

Copy link


Copy Markdown

Contributor
C++ LGTM, @kou any more concerns?
kou
kou
approved these changes
Oct 12, 2021
View reviewed changes
@koukou
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.

I removed the change to vcpkg.json.
Sorry for my poor English skill. It's OK that we add boost-process to vcpkg.json in this pull request. I just wanted to say that there is no problem that we use boost::process for GCS test...
I think that we can merge this with minor Dockerfile related improvements.
Comment threadci/docker/debian-10-cpp.dockerfile
Outdated

Comment threadci/docker/debian-11-cpp.dockerfile
Outdated

Comment threadci/docker/fedora-33-cpp.dockerfile
Outdated

Comment threadci/docker/linux-apt-r.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.04-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-20.10-cpp.dockerfile
Outdated

Comment threadci/docker/ubuntu-21.04-cpp.dockerfile
Outdated

@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g nightly
@github-actions
github-actionsBot
commented
Oct 12, 2021

Copy link


Copy Markdown

Revision: 6aa1b3e36fee6231fe303e0aeafffac416bae6b8
Submitted crossbow builds: ursacomputing/crossbow @ actions-916
TaskStatus
almalinux-8-amd64Github Actions
almalinux-8-arm64TravisCI
amazon-linux-2-amd64Github Actions
centos-7-amd64Github Actions
centos-8-amd64Github Actions
centos-8-arm64TravisCI
conda-cleanAzure
conda-linux-gcc-py36-arm64Azure
conda-linux-gcc-py36-cpu-r40Azure
conda-linux-gcc-py36-cudaAzure
conda-linux-gcc-py37-arm64Azure
conda-linux-gcc-py37-cpu-r41Azure
conda-linux-gcc-py37-cudaAzure
conda-linux-gcc-py38-arm64Azure
conda-linux-gcc-py38-cpuAzure
conda-linux-gcc-py38-cudaAzure
conda-linux-gcc-py39-arm64Azure
conda-linux-gcc-py39-cpuAzure
conda-linux-gcc-py39-cudaAzure
conda-osx-arm64-clang-py38Azure
conda-osx-arm64-clang-py39Azure
conda-osx-clang-py36-r40Azure
conda-osx-clang-py37-r41Azure
conda-osx-clang-py38Azure
conda-osx-clang-py39Azure
conda-win-vs2017-py36-r40Azure
conda-win-vs2017-py37-r41Azure
conda-win-vs2017-py38Azure
conda-win-vs2017-py39Azure
debian-bookworm-amd64Github Actions
debian-bookworm-arm64TravisCI
debian-bullseye-amd64Github Actions
debian-bullseye-arm64TravisCI
debian-buster-amd64Github Actions
debian-buster-arm64TravisCI
example-cpp-minimal-build-staticGithub Actions
example-cpp-minimal-build-static-system-dependencyGithub Actions
homebrew-cppGithub Actions
homebrew-r-autobrewGithub Actions
java-jarsGithub Actions
nugetGithub Actions
python-sdistGithub Actions
test-build-vcpkg-winGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-conda-python-3.6Github Actions
test-conda-python-3.6-pandas-0.23Github Actions
test-conda-python-3.7Github Actions
test-conda-python-3.7-dask-latestGithub Actions
test-conda-python-3.7-dask-masterGithub Actions
test-conda-python-3.7-hdfs-2.9.2Github Actions
test-conda-python-3.7-hdfs-3.2.1Github Actions
test-conda-python-3.7-kartothek-latestGithub Actions
test-conda-python-3.7-kartothek-masterGithub Actions
test-conda-python-3.7-pandas-0.24Github Actions
test-conda-python-3.7-pandas-latestGithub Actions
test-conda-python-3.7-spark-branch-3.0Github Actions
test-conda-python-3.8Github Actions
test-conda-python-3.8-hypothesisGithub Actions
test-conda-python-3.8-pandas-latestGithub Actions
test-conda-python-3.8-pandas-nightlyGithub Actions
test-conda-python-3.8-spark-masterGithub Actions
test-conda-python-3.9Github Actions
test-conda-python-3.9-pandas-masterGithub Actions
test-debian-11-cppGithub Actions
test-debian-11-go-1.15Azure
test-debian-11-python-3Azure
test-debian-c-glibGithub Actions
test-debian-rubyGithub Actions
test-fedora-33-cppGithub Actions
test-fedora-33-python-3Azure
test-r-arrow-backwards-compatibilityGithub Actions
test-r-depsource-autoAzure
test-r-depsource-systemGithub Actions
test-r-devdocsGithub Actions
test-r-gcc-11Github Actions
test-r-install-localGithub Actions
test-r-linux-as-cranGithub Actions
test-r-linux-rchkGithub Actions
test-r-linux-valgrindAzure
test-r-minimal-buildAzure
test-r-offline-maximalGithub Actions
test-r-offline-minimalAzure
test-r-rhub-debian-gcc-devel-lto-latestAzure
test-r-rhub-ubuntu-gcc-release-latestAzure
test-r-rocker-r-base-latestAzure
test-r-rstudio-r-base-3.6-bionicAzure
test-r-rstudio-r-base-3.6-centos7-devtoolset-8Azure
test-r-rstudio-r-base-3.6-centos8Azure
test-r-rstudio-r-base-3.6-opensuse15Azure
test-r-rstudio-r-base-3.6-opensuse42Azure
test-r-ubuntu-21.04Github Actions
test-r-versionsGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-18.04-python-3Azure
test-ubuntu-18.04-r-sanitizerAzure
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-20.10-docsAzure
test-ubuntu-c-glibGithub Actions
test-ubuntu-default-docsAzure
test-ubuntu-rubyGithub Actions
ubuntu-bionic-amd64Github Actions
ubuntu-bionic-arm64TravisCI
ubuntu-focal-amd64Github Actions
ubuntu-focal-arm64TravisCI
ubuntu-hirsute-amd64Github Actions
ubuntu-hirsute-arm64TravisCI
wheel-macos-big-sur-cp39-arm64Github Actions
wheel-macos-big-sur-cp39-universal2Github Actions
wheel-macos-big-sur-py38-arm64Github Actions
wheel-macos-high-sierra-cp36-amd64Github Actions
wheel-macos-high-sierra-cp37-amd64Github Actions
wheel-macos-high-sierra-cp38-amd64Github Actions
wheel-macos-high-sierra-cp39-amd64Github Actions
wheel-macos-mavericks-cp36-amd64Github Actions
wheel-macos-mavericks-cp37-amd64Github Actions
wheel-macos-mavericks-cp38-amd64Github Actions
wheel-macos-mavericks-cp39-amd64Github Actions
wheel-manylinux2010-cp36-amd64Github Actions
wheel-manylinux2010-cp37-amd64Github Actions
wheel-manylinux2010-cp38-amd64Github Actions
wheel-manylinux2010-cp39-amd64Github Actions
wheel-manylinux2014-cp36-amd64Github Actions
wheel-manylinux2014-cp36-arm64TravisCI
wheel-manylinux2014-cp37-amd64Github Actions
wheel-manylinux2014-cp37-arm64TravisCI
wheel-manylinux2014-cp38-amd64Github Actions
wheel-manylinux2014-cp38-arm64TravisCI
wheel-manylinux2014-cp39-amd64Github Actions
wheel-manylinux2014-cp39-arm64TravisCI
wheel-windows-cp36-amd64Github Actions
wheel-windows-cp37-amd64Github Actions
wheel-windows-cp38-amd64Github Actions
wheel-windows-cp39-amd64Github Actions
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
pitrou
pitrou
reviewed
Oct 12, 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 very much @coryan . Here are a number of minor comments.
Comment threadcpp/src/arrow/filesystem/gcsfs.cc
Outdated

@pitroupitrouOct 12, 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.

You can simply write:
ARROW_ASSIGN_OR_RAISE(auto p, GcsPath::FromString(path));
@coryancoryanOct 12, 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.

"simply" ... hmmm ... Done.
Comment threadcpp/src/arrow/filesystem/gcsfs_internal.cc
Outdated

@pitroupitrouOct 12, 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.

I would suggest IOError for all external errors (such as kDeadlineExceeded, kPermissionDenied, kUnauthenticated...).
@coryancoryanOct 12, 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.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Is this sentence incomplete?
@coryancoryanOct 12, 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.

Fixed.
Comment threadcpp/src/arrow/filesystem/gcsfs_test.cc
Outdated

@pitroupitrouOct 12, 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.

Note we have utilities for testing in filesystem/test_util.h that would allow this test to be a bit more interesting, e.g.:
AssertFileInfo(fs.get(), kPreexistingBucket, FileType::Directory);
@coryancoryanOct 12, 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.
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
#0 0x00007fa8e4cc8aff in poll () from /lib/x86_64-linux-gnu/libc.so.6#1 0x00007fa8e542a769 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#2 0x00007fa8e5424342 in ?? () from /lib/x86_64-linux-gnu/libcurl.so.4#3 0x00007fa8e5424536 in curl_multi_poll () from /lib/x86_64-linux-gnu/libcurl.so.4#4 0x00007fa8e541ce24 in curl_easy_perform () from /lib/x86_64-linux-gnu/libcurl.so.4#5 0x00007fa8e83a4b47 in google::cloud::storage::v1::internal::CurlHandle::EasyPerform (this=0x7ffe44de3958)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_handle.h:130#6 0x00007fa8e83a460e in google::cloud::storage::v1::internal::CurlRequest::MakeRequestImpl (this=0x7ffe44de3880)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:125#7 0x00007fa8e83a4031 in google::cloud::storage::v1::internal::CurlRequest::MakeRequest (this=0x7ffe44de3880, payload="")
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/curl_request.cc:77#8 0x00007fa8e84598db in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::DoMetadataServerGetRequest (this=0x55c6ae7c7700, path="/computeMetadata/v1/instance/service-accounts/default/", recursive=true)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:147#9 0x00007fa8e845935b in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::RetrieveServiceAccountInfo (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:158#10 0x00007fa8e845835c in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::Refresh (this=0x55c6ae7c7700) at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:179#11 0x00007fa8e8456a21 in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]()::{lambda()#1}::operator()() const (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#12 0x00007fa8e845874e in google::cloud::storage::v1::oauth2::RefreshingCredentialsWrapper::AuthorizationHeader<google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}>(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader()::{lambda()#1}) const (this=0x55c6ae7c7738, now=..., refresh_fn=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/refreshing_credentials_wrapper.h:49#13 0x00007fa8e8456b2f in google::cloud::storage::v1::oauth2::ComputeEngineCredentials<google::cloud::storage::v1::internal::CurlRequestBuilder, std::chrono::_V2::system_clock>::AuthorizationHeader[abi:cxx11]() (this=0x55c6ae7c7700)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/compute_engine_credentials.h:91#14 0x00007fa8e8453294 in google::cloud::storage::v1::oauth2::GoogleDefaultCredentials (options=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/oauth2/google_credentials.cc:168#15 0x00007fa8e844cf70 in google::cloud::storage::v1::internal::Visitor::visit (this=0x7ffe44de4020)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:45#16 0x00007fa8e8741c8c in google::cloud::v1::internal::GoogleDefaultCredentialsConfig::dispatch (this=0x55c6ae7848d0, v=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.h:71#17 0x00007fa8e874b020 in google::cloud::v1::internal::CredentialsVisitor::dispatch (credentials=..., visitor=warning: RTTI symbol not found for class 'google::cloud::storage::v1::internal::MapCredentials(std::shared_ptr<google::cloud::v1::Credentials> const&)::Visitor'
...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/internal/credentials_impl.cc:24#18 0x00007fa8e844d362 in google::cloud::storage::v1::internal::MapCredentials (
credentials=std::shared_ptr<class google::cloud::v1::Credentials> (use count 1, weak count 0) = {...})
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/internal/unified_rest_credentials.cc:71#19 0x00007fa8e832bd74 in google::cloud::storage::v1::internal::DefaultOptionsWithCredentials (opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client_options.cc:230#20 0x00007fa8e8311112 in google::cloud::storage::v1::Client::Client (this=0x55c6ae7c76d0, opts=...)
at /build/cpp/google_cloud_cpp_ep-prefix/src/google_cloud_cpp_ep/google/cloud/storage/client.cc:41#21 0x00007fa8e7e40a9d in arrow::fs::GcsFileSystem::Impl::Impl (this=0x55c6ae7c7690, o=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:85#22 0x00007fa8e7e45a69 in __gnu_cxx::new_allocator<arrow::fs::GcsFileSystem::Impl>::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (
this=0x7ffe44de439f, __p=0x55c6ae7c7690) at /usr/include/c++/9/ext/new_allocator.h:147#23 0x00007fa8e7e45746 in std::allocator_traits<std::allocator<arrow::fs::GcsFileSystem::Impl> >::construct<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> (__a=..., __p=0x55c6ae7c7690) at /usr/include/c++/9/bits/alloc_traits.h:484#24 0x00007fa8e7e45314 in std::_Sp_counted_ptr_inplace<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<arrow::fs::GcsOptions const&> (this=0x55c6ae7c7680, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:548#25 0x00007fa8e7e44a70 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7670, __p=@0x55c6ae7c7668: 0x0, __a=...) at /usr/include/c++/9/bits/shared_ptr_base.h:679#26 0x00007fa8e7e43f0a in std::__shared_ptr<arrow::fs::GcsFileSystem::Impl, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1344#27 0x00007fa8e7e433a1 in std::shared_ptr<arrow::fs::GcsFileSystem::Impl>::shared_ptr<std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (
this=0x55c6ae7c7668, __tag=...) at /usr/include/c++/9/bits/shared_ptr.h:359#28 0x00007fa8e7e42a33 in std::allocate_shared<arrow::fs::GcsFileSystem::Impl, std::allocator<arrow::fs::GcsFileSystem::Impl>, arrow::fs::GcsOptions const&> (__a=...)
at /usr/include/c++/9/bits/shared_ptr.h:702#29 0x00007fa8e7e41ad3 in std::make_shared<arrow::fs::GcsFileSystem::Impl, arrow::fs::GcsOptions const&> () at /usr/include/c++/9/bits/shared_ptr.h:718#30 0x00007fa8e7e3ed70 in arrow::fs::GcsFileSystem::GcsFileSystem (this=0x55c6ae7c7620, options=..., context=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:202#31 0x00007fa8e7e3ede8 in arrow::fs::internal::MakeGcsFileSystemForTest (options=...) at /arrow/cpp/src/arrow/filesystem/gcsfs.cc:209#32 0x000055c6ac5fca88 in arrow::fs::(anonymous namespace)::GcsFileSystem_FileSystemCompare_Test::TestBody (this=0x55c6ae784800)
at /arrow/cpp/src/arrow/filesystem/gcsfs_test.cc:145#33 0x00007fa8e55d1634 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#34 0x00007fa8e55c932b in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x55c6ae784800, method=&virtual testing::Test::TestBody(), location=0x7fa8e55e857b "the test body") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#35 0x00007fa8e559d5ec in testing::Test::Run (this=0x55c6ae784800) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2682#36 0x00007fa8e559e02e in testing::TestInfo::Run (this=0x55c6ae7c5870) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2861#37 0x00007fa8e559e948 in testing::TestSuite::Run (this=0x55c6ae7c5470) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:3015#38 0x00007fa8e55ae3f6 in testing::internal::UnitTestImpl::RunAllTests (this=0x55c6ae7c51d0) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5855#39 0x00007fa8e55d2b62 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2607#40 0x00007fa8e55ca569 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x55c6ae7c51d0, method=(bool (testing::internal::UnitTestImpl::*)(class testing::internal::UnitTestImpl * const)) 0x7fa8e55adfc2 <testing::internal::UnitTestImpl::RunAllTests()>, location=0x7fa8e55e9068 "auxiliary test code (environments or event listeners)") at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:2643#41 0x00007fa8e55acb0d in testing::UnitTest::Run (this=0x7fa8e5616ba0 <testing::UnitTest::GetInstance()::instance>) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest.cc:5438#42 0x00007fa8e561829a in RUN_ALL_TESTS () at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/include/gtest/gtest.h:2490#43 0x00007fa8e561821c in main (argc=1, argv=0x7ffe44de4ba8) at /build/cpp/googletest_ep-prefix/src/googletest_ep/googletest/src/gtest_main.cc:52#44 0x00007fa8e4bda0b3 in __libc_start_main () from /lib/x86_64-linux-gnu/libc.so.6#45 0x000055c6ac5fb0de in _start ()
@coryan
coryan
commented
Oct 12, 2021

Copy link


Copy Markdown

ContributorAuthor
Can we set ARROW_GCS=ON like ARROW_S3=ON in our CI?
Which ones? Some of them already have these settings, e.g.:
https://github.com/apache/arrow/pull/11375/checks?check_run_id=3863850315#step:8:2266
How large is the slowdown? Compiling embedded libraries from scratch adds to CI delays.
That obviously depends on how much caching is going on and what the CI machine specs are. For reference, I used vcpkg and only 4 cores to "benchmark" things. I can build google-cloud-cpp[core,storage] in 3 minutes (both the debug and release). Its dependencies take another 3 minutes, most likely you already have some of those (e.g. zlib, curl, openssl, and even abseil for gRPC).
@pitrou
pitrou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
The tests hang here in GcsFileSystem.FileSystemCompare. Here is the gdb backtrace:
I've now run strace on the test process and it seems it spawns a child process that tries to resolve metadata.google.internal. That's where it hangs.
@kou
kou
commented
Oct 12, 2021

Copy link


Copy Markdown

Member
Some of them already have these settings
Oh, sorry. I missed them.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I think I addressed all the comments, please let me know if I missed something.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

pitrou
pitrou
approved these changes
Oct 13, 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.

+1 (but would be nice to eliminate the slowness in GcsFileSystem.FileSystemCompare)
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
Hmm, gcfs_test is not hanging anymore but it's still taking a lot of time:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (20025 ms)

Ugh, that looks painful. On my workstation that particular test completes in milliseconds:
[ RUN ] GcsFileSystem.FileSystemCompare
[ OK ] GcsFileSystem.FileSystemCompare (5 ms)

And the full test with with archery docker run ubuntu-cpp takes about 3 seconds:
54/85 Test #49: arrow-gcsfs-test .......................... Passed 3.05 sec

Which is to say: this would probably take some more effort to troubleshoot. I will open a separate bug.
@coryan
coryan
commented
Oct 13, 2021

Copy link


Copy Markdown

ContributorAuthor
I will open a separate bug.
https://issues.apache.org/jira/browse/ARROW-14311
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
Thank you, I'll merge this one then.
@pitrou
pitrou
commented
Oct 13, 2021

Copy link


Copy Markdown

Member
@github-actions crossbow submit -g cpp
@github-actions
github-actionsBot
commented
Oct 13, 2021

Copy link


Copy Markdown

Revision: f3edc48
Submitted crossbow builds: ursacomputing/crossbow @ actions-935
TaskStatus
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-debian-11-cppGithub Actions
test-fedora-33-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-14Github Actions
test-ubuntu-20.04-cpp-17Github Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
@pitroupitrou
changed the title
ARROW-14236: [C++] add GCS testbench for testingARROW-14236: [C++] Add GCS testbench for testingOct 13, 2021
@ursabot
ursabot
commented
Oct 13, 2021•

edited

Loading

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 42b0665 and contender = 74caee2. 74caee2 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

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.85% ⬆️0.0%] 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-14236-install-gcs-testbench

branch
October 13, 2021 18:14
@asfimportasfimport
mentioned this pull request
Oct 14, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@emkornfieldemkornfieldemkornfield left review comments
@koukoukou approved these changes
@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.

5 participants

@coryan@emkornfield@kou@pitrou@ursabot