-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(native): Fix executor name in PrestoExchangeSource #26809
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
base: master
Are you sure you want to change the base?
fix(native): Fix executor name in PrestoExchangeSource #26809
Conversation
In commit 74fb8ce, a separate thread pool(See `TaskServer::exchangeHttpCpuExecutor_`) was introduced, replacing the driver thread pool to process exchange requests. However, the name `driverExecutor_` is still being used in `PrestoExchangeSource`, which is misleading. This PR fixes this issue.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRenames the PrestoExchangeSource executor field and all its usages from a misleading driverExecutor_ to the correct exchangeHttpCpuExecutor_ to reflect that exchange HTTP requests run on a dedicated thread pool, not the driver pool. Updated class diagram for PrestoExchangeSource executor renameclassDiagram
class ExchangeSource
class PrestoExchangeSource {
+PrestoExchangeSource(
std::shared_ptr<http::HttpClient> httpClient,
folly::CPUThreadPoolExecutor* exchangeHttpCpuExecutor,
bool enableBufferCopy,
bool immediateBufferTransfer
)
-bool enableBufferCopy_
-bool immediateBufferTransfer_
-folly::CPUThreadPoolExecutor* exchangeHttpCpuExecutor_
-std::shared_ptr<http::HttpClient> httpClient_
-RetryState dataRequestRetryState_
}
ExchangeSource <|-- PrestoExchangeSource
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Now that the member has been renamed to
exchangeHttpCpuExecutor_, consider also renaming the constructor parameter (and any remaining references) fromdriverExecutorto something likeexchangeHttpCpuExecutorfor consistency and to avoid future confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that the member has been renamed to `exchangeHttpCpuExecutor_`, consider also renaming the constructor parameter (and any remaining references) from `driverExecutor` to something like `exchangeHttpCpuExecutor` for consistency and to avoid future confusion.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aditi-pandit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lingbin for the code cleanup
In commit 74fb8ce, a separate thread pool(See
TaskServer::exchangeHttpCpuExecutor_) was introduced, replacing thedriver thread pool to process exchange requests. However, the name
driverExecutor_is still being used inPrestoExchangeSource,which is misleading.
This PR fixes this issue.