-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: Add max_split_preload_per_driver to java system config #26583
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?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces a new session configuration property max_split_preload_per_driver to control how many splits are preloaded per driver, by adding its constant, registering it in the session properties constructor, and providing a getter. Class diagram for updated SystemSessionProperties with max_split_preload_per_driverclassDiagram
class SystemSessionProperties {
+String MAX_SPLIT_PRELOAD_PER_DRIVER
+int getMaxSplitPreloadPerDriver(Session session)
}
SystemSessionProperties --> Session : uses
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/SystemSessionProperties.java:194` </location>
<code_context>
public static final String IGNORE_STATS_CALCULATOR_FAILURES = "ignore_stats_calculator_failures";
public static final String PRINT_STATS_FOR_NON_JOIN_QUERY = "print_stats_for_non_join_query";
public static final String MAX_DRIVERS_PER_TASK = "max_drivers_per_task";
+ public static final String MAX_SPLIT_PRELOAD_PER_DRIVER = "max_split_preload_per_driver";
public static final String MAX_TASKS_PER_STAGE = "max_tasks_per_stage";
public static final String DEFAULT_FILTER_FACTOR_ENABLED = "default_filter_factor_enabled";
</code_context>
<issue_to_address>
**suggestion:** Consider validating the value for MAX_SPLIT_PRELOAD_PER_DRIVER.
This property currently lacks validation for acceptable values. Please ensure it cannot be set to negative or unreasonably high numbers, following the approach used for similar properties.
Suggested implementation:
```java
integerProperty(
MAX_SPLIT_PRELOAD_PER_DRIVER,
"Maximum number of splits to preload per driver. Set to 0 to disable preloading",
0,
false,
value -> min(taskManagerConfig.getMaxDriversPerTask(), validateNullablePositiveIntegerValue(value, MAX_SPLIT_PRELOAD_PER_DRIVER)),
object -> object),
```
If `validateNullablePositiveIntegerValue` or a similar helper does not exist for this property, you may need to:
1. Ensure that the helper method is available and imported.
2. Adjust the upper bound as appropriate for your system (e.g., define a new constant if `taskManagerConfig.getMaxDriversPerTask()` is not suitable).
3. If the property registration method signature differs, adapt the validation lambda accordingly.
</issue_to_address>
### Comment 2
<location> `presto-main-base/src/main/java/com/facebook/presto/SystemSessionProperties.java:1030` </location>
<code_context>
object -> object),
+ integerProperty(
+ MAX_SPLIT_PRELOAD_PER_DRIVER,
+ "Maximum number of splits to preload per driver. Set to 0 to disable preloading",
+ 0,
+ false),
</code_context>
<issue_to_address>
**suggestion:** Clarify behavior for negative values in the property description.
If negative values are invalid, please update the description to explicitly state this.
```suggestion
"Maximum number of splits to preload per driver. Set to 0 to disable preloading. Negative values are not allowed.",
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-main-base/src/main/java/com/facebook/presto/SystemSessionProperties.java
Show resolved
Hide resolved
652646d to
04eedad
Compare
…m config (prestodb#26583) Summary: as title Reviewed By: kewang1024 Differential Revision: D86237359
04eedad to
e5133ee
Compare
…m config (prestodb#26583) Summary: as title Reviewed By: kewang1024 Differential Revision: D86237359
e5133ee to
e684f45
Compare
…ig (prestodb#26583) Summary: as title Reviewed By: kewang1024 Differential Revision: D86237359
e684f45 to
69e502f
Compare
Summary: as title Reviewed By: kewang1024 Differential Revision: D86237359
69e502f to
4e1c29e
Compare
…b#26583) Summary: as title Reviewed By: kewang1024 Differential Revision: D86237359
4e1c29e to
ac77a92
Compare
…b#26583) Summary: as title Reviewed By: kewang1024 Differential Revision: D86237359
ac77a92 to
485d38f
Compare
|
Please add documentation for |
Summary: as title
Reviewed By: kewang1024
Differential Revision: D86237359