Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-14311: [C++] Make GCS FileSystem tests faster #11406

Closed
wants to merge 1 commit into from
Closed

ARROW-14311: [C++] Make GCS FileSystem tests faster #11406

wants to merge 1 commit into from

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Oct 13, 2021

Avoid using the default credentials for the unit test. In some
environments these can be very slow to initialize, as they test several
different potential sources of credentials, some including HTTP
requests.

@github-actions
Copy link

@coryan coryan marked this pull request as ready for review October 13, 2021 19:06
@coryan
Copy link
Contributor Author

coryan commented Oct 13, 2021

@pitrou would you be able to test this in your environment? I have good reason to believe (strace output) that this fixes the problem. An independent check would be helpful, as it was working well in my environment.

@pitrou
Copy link
Member

pitrou commented Oct 15, 2021

It works indeed :-)

[==========] Running 5 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 4 tests from GcsFileSystem
[ RUN      ] GcsFileSystem.OptionsCompare
[       OK ] GcsFileSystem.OptionsCompare (0 ms)
[ RUN      ] GcsFileSystem.ToArrowStatusOK
[       OK ] GcsFileSystem.ToArrowStatusOK (0 ms)
[ RUN      ] GcsFileSystem.ToArrowStatus
[       OK ] GcsFileSystem.ToArrowStatus (0 ms)
[ RUN      ] GcsFileSystem.FileSystemCompare
[       OK ] GcsFileSystem.FileSystemCompare (2 ms)
[----------] 4 tests from GcsFileSystem (2 ms total)

[----------] 1 test from GcsIntegrationTest
[ RUN      ] GcsIntegrationTest.MakeBucket
INFO:werkzeug: * Running on http://localhost:60709/ (Press CTRL+C to quit)
INFO:werkzeug: * Restarting with stat
INFO:werkzeug:127.0.0.1 - - [15/Oct/2021 19:00:25] "POST /storage/v1/b?project=ignored-by-testbench HTTP/1.1" 200 -
INFO:werkzeug:127.0.0.1 - - [15/Oct/2021 19:00:25] "GET /storage/v1/b/test-bucket-name HTTP/1.1" 200 -
[       OK ] GcsIntegrationTest.MakeBucket (1551 ms)
[----------] 1 test from GcsIntegrationTest (1551 ms total)

[----------] Global test environment tear-down
[==========] 5 tests from 2 test suites ran. (1554 ms total)
[  PASSED  ] 5 tests.

@pitrou
Copy link
Member

pitrou commented Oct 15, 2021

(1.5s make still be a bit long once there are a lot of tests, but perhaps it's not really possible to make it faster)

@coryan
Copy link
Contributor Author

coryan commented Oct 15, 2021

(1.5s make still be a bit long once there are a lot of tests, but perhaps it's not really possible to make it faster)

I will see what I tolerate. At some point I might start the testbench once, and then isolate the tests using different buckets or something similar.

Avoid using the default credentials for the unit test. In some
environments these can be very slow to initialize, as they test several
different potential sources of credentials, some including HTTP
requests.
@coryan
Copy link
Contributor Author

coryan commented Oct 20, 2021

Is there something else I need to do here or can we merge this PR?

@coryan
Copy link
Contributor Author

coryan commented Oct 26, 2021

Ping. Is there are reason we cannot merge this PR?

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

I'll merge this.

@pitrou If you have any concern, please write it here.

@kou kou closed this in 1b7178d Oct 26, 2021
@ursabot
Copy link

ursabot commented Oct 26, 2021

Benchmark runs are scheduled for baseline = 1bac505 and contender = 1b7178d. 1b7178d 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.51% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.18%] 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

@pitrou
Copy link
Member

pitrou commented Oct 27, 2021

@coryan Sorry for the delay. I was on holiday.

@coryan coryan deleted the ARROW-14311-GCS-filesystem-now-uses-insecure-credentials-for-http branch October 27, 2021 12:48
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Nov 3, 2021
Avoid using the default credentials for the unit test. In some
environments these can be very slow to initialize, as they test several
different potential sources of credentials, some including HTTP
requests.

Closes apache#11406 from coryan/ARROW-14311-GCS-filesystem-now-uses-insecure-credentials-for-http

Authored-by: Carlos O'Ryan <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants