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

[ISSUE #4731] HttpRequestProcessor enhancement #4732

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

karsonto
Copy link
Contributor

@karsonto karsonto commented Jan 9, 2024

Fixes #4731.

Motivation

Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.

Modifications

Describe the modifications you've done.

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Comment on lines 71 to 75
/**
*
* @return {@link Executor}
*/
Executor executor();

Copy link
Member

Choose a reason for hiding this comment

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

It seems this method has no functional relation with HttpRequestProcessor interface. As for the current implementation of EventMesh for asynchronous requests, the executor has not been placed within the interface that handles the requests. May you please explain its design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because when I was looking at the source code, I found that the HttpRequestProcessor requires a thread pool to execute, and this design can clearly know which thread pool executes the processor .

Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

@karsonto Please fix ci error

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (648f3e9) 0.00% compared to head (da04b07) 0.00%.

❗ Current head da04b07 differs from pull request most recent head 7ebdf35. Consider uploading reports for the commit 7ebdf35 to get more accurate results

Additional details and impacted files
@@      Coverage Diff       @@
##   master   #4732   +/-   ##
==============================
==============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Alonexc
Copy link
Contributor

Alonexc commented Jan 16, 2024

Conflicts need to be resolved.

@karsonto karsonto force-pushed the develop-httpProcess-enhancement branch from df92b6f to da04b07 Compare January 16, 2024 09:45
@karsonto
Copy link
Contributor Author

karsonto commented Jan 16, 2024

Conflicts need to be resolved.

@Alonexc Please help to review again

@xwm1992
Copy link
Contributor

xwm1992 commented Feb 5, 2024

@karsonto please resolve the conflicts thanks.

@karsonto karsonto force-pushed the develop-httpProcess-enhancement branch from da04b07 to 7ebdf35 Compare February 5, 2024 03:29
@karsonto
Copy link
Contributor Author

karsonto commented Feb 5, 2024

@xwm1992 ok,please review.

@Pil0tXia Pil0tXia merged commit 9a3912a into apache:master Feb 18, 2024
9 checks passed
@xwm1992 xwm1992 added this to the 1.11.0 milestone Dec 18, 2024
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.

[Enhancement] Optimize the HttpRequestProcessor can obtain executor
5 participants