HADOOP-18890: remove use of okhttp in runtime code #6057
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@rohit-kb what do you think of this? would it suit? |
steveloughran
requested changes
Sep 13, 2023
| "Received invalid http response: " + statusCode + ", text = " + | ||
| EntityUtils.toString(response.getEntity())); | ||
| } | ||
| Map<?, ?> responseBody = JsonSerialization.mapReader().readValue( |
There was a problem hiding this comment.
let's check the return content type too; been burned by both proxies and how the abfs oauth failure is 200 + text/html for users.
There was a problem hiding this comment.
The pre-existing code did not check the content-type of the return. It is not uncommon for an API to return JSON but not to have application/json Content-Type. I can make the change but I'm worried that the API we call may not set the expected content-type on the response.
|
|
||
| accessToken = responseBody.get(ACCESS_TOKEN).toString(); | ||
| } catch (RuntimeException e) { | ||
| throw new IOException("Unable to obtain access token from credential", e); |
There was a problem hiding this comment.
add e.toString to the text
There was a problem hiding this comment.
I'm duplicating this because Spotbugs has its weird rules. The pre-existing catch already does this - do I change all the catches?
|
|
||
| accessToken = responseBody.get(ACCESS_TOKEN).toString(); | ||
| } catch (RuntimeException e) { | ||
| throw new IOException("Unable to obtain access token from credential", e); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
this is now a duplicate of the handler above, isn't it? so they can be collapsed
There was a problem hiding this comment.
Spotbugs is what is making me duplicate the catch. It doesn't like catches of Exception and this is one workaround.
pjfanning
changed the title
|
Thanks for the heads-up @steveloughran. I think it's a welcome change w.r.t to okhttp and related kotlin dependencies removal. Just need to check if it's causing any issues in any other components downstream. Thanks @pjfanning for looking into this. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
pjfanning
force-pushed
the
HADOOP-18496-okhttp
branch
from
e70ad48
to
0bb3e35
Compare
steveloughran
requested changes
Sep 15, 2023
| @@ -391,11 +391,19 @@ | |||
| <dependency> | |||
| <groupId>com.squareup.okhttp3</groupId> | |||
| <artifactId>mockwebserver</artifactId> | |||
| <version>${okhttp3.version}</version> | |||
There was a problem hiding this comment.
prefer hadoop-project retains the declaration, but now with all the excludes. the modules should get the exclusions automatically, shouldn't they?
| @@ -220,62 +218,6 @@ | |||
|
|
|||
| <dependencyManagement> | |||
| <dependencies> | |||
| <dependency> | |||
There was a problem hiding this comment.
this should be reinstated with the new exclusions; then the test declarations don't need changing.
There was a problem hiding this comment.
I've committed some changes to bring back some of dependency management to this pom
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
test failures are unrelated; the timeline one has been seen before YARN-11546 |
steveloughran
approved these changes
Sep 19, 2023
|
+1 merged to trunk. presumably it needs a backport? |
pjfanning
added a commit
to pjfanning/hadoop
that referenced
this pull request
Sep 19, 2023
Contributed by PJ Fanning
|
I created #6101 for backporting this. |
pjfanning
added a commit
to pjfanning/hadoop
that referenced
this pull request
Sep 26, 2023
Contributed by PJ Fanning
4 tasks
pjfanning
added a commit
to pjfanning/hadoop
that referenced
this pull request
Oct 11, 2023
Contributed by PJ Fanning
steveloughran
pushed a commit
that referenced
this pull request
Oct 13, 2023
Contributed by PJ Fanning
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
Contributed by PJ Fanning
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Use Apache HTTPClient instead of OkHTTP to reduce the number of hadoop dependencies.
https://issues.apache.org/jira/browse/HADOOP-18890
How was this patch tested?
Local build and CI build.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?