Skip to content

Conversation

@dnovitski
Copy link
Contributor

Summary

Implements issue #1401 to give all threads a name

Description

  • Replaced Executors.newXXX with new ExecutorFactory util which takes thread name argument

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

return Executors.newFixedThreadPool(nThreads, getThreadFactory(threadName));
}

private static ThreadFactory getThreadFactory(String threadName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There're few places where similar monitoring threads are opened against different nodes. It seems that those threads will have the same name. Any chance to avoid using the same name for different threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

I've added a static thread factory map so threads with the same name will use the same thread factory which has an atomic counter for each new thread created to differentiate them

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of driver logs include thread names and the only concern here is to make them less readable. Is there a chance to come up to a shorter thread names? Potentially we can completely rid of class name. Corresponding class name and method could mentioned in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point as well. I've removed the classname# part of the thread name and shortened it while still keeping some descriptive name for it.

@dnovitski dnovitski requested a review from sergiyvamz May 21, 2025 10:26
@dnovitski
Copy link
Contributor Author

Example screenshot from Visual VM

Screenshot 2025-05-21 at 13 15 09

@dnovitski
Copy link
Contributor Author

Checkstyle issues should be fixed now

@sergiyvamz sergiyvamz merged commit 957cbe2 into aws:main May 27, 2025
6 checks passed
@sergiyvamz
Copy link
Contributor

@dnovitski Thank you for your efforts making the driver better!

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.

3 participants