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

Enforce early socket read timeout setting. #866

Conversation

cnauroth
Copy link
Member

Set socket read timeout (fs.gs.http.read-timeout) as early as possible
on new sockets returned from the custom SSLSocketFactory. This
guarantees the timeout is enforced during TLS handshakes when using
Conscrypt as the security provider.

See also google/conscrypt#864 .

(cherry picked from commit 61f8629)
(cherry picked from commit 667836b)

Set socket read timeout (`fs.gs.http.read-timeout`) as early as possible
on new sockets returned from the custom `SSLSocketFactory`. This
guarantees the timeout is enforced during TLS handshakes when using
Conscrypt as the security provider.

See also google/conscrypt#864 .

(cherry picked from commit 61f8629)
(cherry picked from commit 667836b)
@cnauroth
Copy link
Member Author

/gcbrun

@cnauroth
Copy link
Member Author

Hello @Deependra-Patel and @medb . The backport to branch-2.1.x is somewhat different due to the presence of Apache HTTP Client support.

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #866 (667836b) into branch-2.1.x (3dba4ed) will decrease coverage by 1.08%.
The diff coverage is 50.46%.

❗ Current head 667836b differs from pull request most recent head c2cde85. Consider uploading reports for the commit c2cde85 to get more accurate results

@@                Coverage Diff                 @@
##             branch-2.1.x     #866      +/-   ##
==================================================
- Coverage           81.79%   80.71%   -1.09%     
- Complexity           1979     2163     +184     
==================================================
  Files                 128      150      +22     
  Lines                8607     9819    +1212     
  Branches             1005     1133     +128     
==================================================
+ Hits                 7040     7925     +885     
- Misses               1145     1418     +273     
- Partials              422      476      +54     
Flag Coverage Δ
hadoop2integrationtest 60.95% <43.03%> (+1.03%) ⬆️
hadoop2unittest 68.56% <47.33%> (-0.92%) ⬇️
hadoop3integrationtest 60.82% <43.38%> (+0.91%) ⬆️
hadoop3unittest 68.62% <47.67%> (-0.87%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...doop/io/bigquery/AbstractExportToCloudStorage.java 77.55% <0.00%> (-1.62%) ⬇️
...ud/hadoop/io/bigquery/AvroBigQueryInputFormat.java 100.00% <ø> (ø)
...gle/cloud/hadoop/io/bigquery/AvroRecordReader.java 81.39% <ø> (ø)
...gle/cloud/hadoop/io/bigquery/ExportFileFormat.java 100.00% <ø> (ø)
...oop/io/bigquery/HadoopCredentialConfiguration.java 0.00% <0.00%> (ø)
...adoop/io/bigquery/JsonTextBigQueryInputFormat.java 0.00% <ø> (ø)
.../cloud/hadoop/io/bigquery/UnshardedInputSplit.java 100.00% <ø> (ø)
...o/bigquery/output/BigQueryOutputConfiguration.java 86.77% <ø> (ø)
...oop/io/bigquery/samples/WikipediaRequestBytes.java 0.00% <0.00%> (ø)
...va/com/google/cloud/hadoop/fs/gcs/FsBenchmark.java 0.00% <0.00%> (ø)
... and 161 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cnauroth
Copy link
Member Author

/gcbrun

@cnauroth
Copy link
Member Author

I'm not sure what's going on with codecov/patch. It seems to be reporting missing coverage on lines that are unchanged in this pull request. When I try to click through to the Codecov site, I get this error:

Changes found in between 3dba4ed...eefc2ff (pseudo...base) which prevent comparing this pull request.

@cnauroth cnauroth merged commit 7939846 into GoogleCloudDataproc:branch-2.1.x Sep 6, 2022
@cnauroth cnauroth deleted the read-timeout-branch-2.1.x branch September 6, 2022 16:30
@cnauroth
Copy link
Member Author

cnauroth commented Sep 6, 2022

@medb , thank you for the review. I have merged this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants