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

[INLONG-4203][Manager] Improve HTTP request and response parse in manager client #4204

Merged
merged 2 commits into from
May 16, 2022

Conversation

leosanqing
Copy link
Contributor

@leosanqing leosanqing commented May 14, 2022

Title Name: [INLONG-4203][Manager] Improve http request and response parse in manager client

Fixes #4203

Motivation

  • The stream of the HTTP request is not closed, which may cause a leak

  • When creating an OkHttp request, there is redundant code. After the get() param, post() is used, and it can also be simplified.

  • When parsing the response, the processing is a bit cumbersome, and the generic type is not used

Modifications

  • Use try-with-resources to prevent unclosed streams

  • Provide a new JSON parsing method to easily parse the response of HTTP requests

  • Correct some wrong usage of creating OkHttp Request object

Verifying this change

  • This change is a trivial rework/code cleanup without any test coverage.

Documentation

  • Does this pull request introduces a new feature? (no)

@healchow healchow changed the title [INLONG-4203] [Manager] Improve http request and response parse #4203 [INLONG-4203][Manager] Improve http request and response parse in manager client May 14, 2022
@healchow
Copy link
Member

@shink The InLong Greeting / Greet process was failed, PTAL.

@healchow
Copy link
Member

@kipshi This PR involves the manager client, PTAL.

@healchow healchow changed the title [INLONG-4203][Manager] Improve http request and response parse in manager client [INLONG-4203][Manager] Improve HTTP request and response parse in manager client May 14, 2022
@shink
Copy link
Member

shink commented May 14, 2022

@healchow Please check out actions/first-interaction#31 (comment). We need pull_request_target for it to work.

@shink
Copy link
Member

shink commented May 14, 2022

@healchow Fixed in #4206, please review.
@leosanqing Thank you for your contribution. Please rebase your commits after #4206 be merged.

@healchow
Copy link
Member

#4206 was merged. @leosanqing Please rebase the master branch into your PR, thanks.

@github-actions github-actions bot added service/ci Automatically confirm that the code is error-free component/sort labels May 16, 2022
Copy link
Member

@healchow healchow left a comment

Choose a reason for hiding this comment

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

I found that the rebase occurred with some unexpected changes, please reset from the master branch, and commit again. Thanks a lot.

@github-actions github-actions bot removed service/ci Automatically confirm that the code is error-free component/sort labels May 16, 2022
Copy link
Member

@healchow healchow left a comment

Choose a reason for hiding this comment

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

LGTM

@healchow healchow merged commit 0553b32 into apache:master May 16, 2022
vernedeng pushed a commit to vernedeng/incubator-inlong that referenced this pull request May 19, 2022
vernedeng pushed a commit to vernedeng/incubator-inlong that referenced this pull request Jul 4, 2022
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.

[Improve][Manager] Improve the HTTP request and response parse
4 participants