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.

Sign up for GitHub

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

Jump to bottom

[ISSUE #4674] Add a run log for the ThreadPoolExecutor #4673

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[ISSUE #4674] Add a run log for the ThreadPoolExecutor #4673

wants to merge 2 commits into from

Conversation

karsonto
Copy link
Contributor

@karsonto karsonto commented Dec 18, 2023 •
edited

Fixes #4674

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

@codecov Codecov
Copy link

codecov bot commented Dec 18, 2023 •
edited

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (958b041) 17.28% compared to head (ae0b95f) 17.37%.
Report is 15 commits behind head on master.

Files Patch % Lines
...apache/eventmesh/common/LogThreadPoolExecutor.java 0.00% 26 Missing ⚠️
...org/apache/eventmesh/common/ThreadPoolFactory.java 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #4673      +/-   ##
============================================
+ Coverage     17.28%   17.37%   +0.09%     
- Complexity     1742     1759      +17     
============================================
  Files           792      798       +6     
  Lines         29703    29821     +118     
  Branches       2567     2581      +14     
============================================
+ Hits           5134     5182      +48     
- Misses        24095    24158      +63     
- Partials        474      481       +7

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

mxsm
mxsm reviewed Dec 18, 2023
View reviewed changes
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 Thanks for your contribution. Please create a issue to describe you idea and link this pr to it.

@karsonto
Copy link
Contributor Author

karsonto commented Dec 18, 2023 •
edited

@mxsm thanks for your reminder，I created issue and link this PR.

@pandaapo pandaapo changed the title [Enhancement] Logging for ThreadPoolExecutor [Enhancement] Add a run log for the ThreadPoolExecutor Dec 18, 2023
Pil0tXia
Pil0tXia reviewed Dec 19, 2023
View reviewed changes
eventmesh-common/src/main/java/org/apache/eventmesh/common/ThreadPoolFactory.java
Comment on lines 41 to +44

public static ThreadPoolExecutor createThreadPoolExecutor(int core, int max, BlockingQueue<Runnable> blockingQueue,
final String threadName, final boolean isDaemon) {
return new ThreadPoolExecutor(core, max, 10 * 1000, TimeUnit.MILLISECONDS, blockingQueue,
return new LogThreadPoolExecutor(core, max, 10 * 1000, TimeUnit.MILLISECONDS, blockingQueue,
Copy link
Member

Choose a reason for hiding this comment

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

These two createThreadPoolExecutor methods are used by EventMeshGrpcServer and HTTPThreadPoolGroup. Why their return type should be changed to LogThreadPoolExecutor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning the LogThreadPoolExecutor can lay the groundwork for subsequent monitoring.

Copy link
Member

Choose a reason for hiding this comment

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

May you please describe the subsequent monitoring features to be implemented?

mxsm
mxsm reviewed Dec 21, 2023
View reviewed changes
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 Can you elaborate on the subsequent use of this design? For example, how and where it will be used in detail?

@karsonto
Copy link
Contributor Author

@karsonto Can you elaborate on the subsequent use of this design? For example, how and where it will be used in detail?

I have reviewed the source code and found that this class is used in many places. Using this class allows you to print exception information in the log of the thread and also to print information about closing the thread pool.

@xwm1992 xwm1992 changed the title [Enhancement] Add a run log for the ThreadPoolExecutor [ISSUE #4674] Add a run log for the ThreadPoolExecutor Dec 22, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 5, 2024

It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback.

If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh.

@github-actions github-actions bot added the Stale label Apr 5, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 17.37%. Comparing base (958b041) to head (ae0b95f).
Report is 72 commits behind head on master.

Files Patch % Lines
...apache/eventmesh/common/LogThreadPoolExecutor.java 0.00% 26 Missing ⚠️
...org/apache/eventmesh/common/ThreadPoolFactory.java 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #4673      +/-   ##
============================================
+ Coverage     17.28%   17.37%   +0.09%     
- Complexity     1742     1759      +17     
============================================
  Files           792      798       +6     
  Lines         29703    29821     +118     
  Branches       2567     2581      +14     
============================================
+ Hits           5134     5182      +48     
- Misses        24095    24158      +63     
- Partials        474      481       +7

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

@github-actions github-actions bot removed the Stale label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxsm mxsm mxsm left review comments

@pandaapo pandaapo pandaapo approved these changes

@Pil0tXia Pil0tXia Awaiting requested review from Pil0tXia

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement] Add a run log for the ThreadPoolExecutor
5 participants
@karsonto @codecov-commenter @mxsm @pandaapo @Pil0tXia