You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "J-HowHuang (via GitHub)" <gi...@apache.org> on 2025/06/18 20:59:37 UTC
[PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
J-HowHuang opened a new pull request, #16144:
URL: https://github.com/apache/pinot/pull/16144
## Overview
Added support for overriding the default disk utilization threshold during table rebalance operations, allowing users to customize disk space checks on a per-rebalance basis.
### Backend Changes
- **RebalanceConfig**: Added `diskUtilizationThreshold` field (double, default: -1.0)
- **Rebalance API**: New query parameter `diskUtilizationThreshold` in `POST /tables/{tableName}/rebalance`
- **Pre-checker**: Updated `DefaultRebalancePreChecker` to use override threshold when provided
- **Variable Rename**: `_diskUtilizationThreshold` → `_defaultDiskUtilizationThreshold` for clarity in `DefaultRebalancePreChecker`
### API Usage
```bash
POST /tables/myTable/rebalance?type=OFFLINE&diskUtilizationThreshold=0.85
```
### UI Changes
- Added "Disk Utilization Threshold" field to Advanced Options in rebalance dialog
- New DOUBLE input component for decimal values (0.0-1.0 range)
### Behavior
- **Default (-1.0)**: Uses controller's configured threshold (the approach before this PR)
- **Override (>=0.0)**: Uses specified threshold (e.g., 0.85 = 85%)
## Rebalance UI
When the value is default (-1.0)
<img width="224" alt="image" src="https://github.com/user-attachments/assets/b65eb0cc-3360-41bc-bd79-045b48fae263" />
When the value is 0.2
When the value is 15 (no check on this value, though)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16144:
URL: https://github.com/apache/pinot/pull/16144#discussion_r2155731917
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/RebalanceServer/RebalanceServerOptions.ts:
##########
@@ -174,5 +174,15 @@ export const rebalanceServerOptions: RebalanceServerOption[] = [
"isAdvancedConfig": true,
"isStatsGatheringConfig": false,
"markWithWarningIcon": false
+ },
+ {
+ "name": "diskUtilizationThreshold",
+ "defaultValue": -1.0,
+ "type": "DOUBLE",
+ "label": "Disk Utilization Threshold",
+ "description": "Override disk utilization threshold used in pre-check (0.0 to 1.0, e.g., 0.85 for 85%). If not provided (or -1.0), uses the controller's default threshold",
+ "isAdvancedConfig": true,
+ "isStatsGatheringConfig": false,
Review Comment:
this should be marked as true right? since it's only used for pre-check? (i forget how this is used in the UI - good to check once)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16144:
URL: https://github.com/apache/pinot/pull/16144#discussion_r2167385991
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -673,6 +673,9 @@ public RebalanceResult rebalance(
@QueryParam("retryInitialDelayInMs") long retryInitialDelayInMs,
@ApiParam(value = "Whether to update segment target tier as part of the rebalance") @DefaultValue("false")
@QueryParam("updateTargetTier") boolean updateTargetTier,
+ @ApiParam(value = "Disk utilization threshold override used in pre-check (0.0 to 1.0, e.g., 0.85 for 85%). "
+ + "If not provided, uses the controller's default threshold") @DefaultValue("-1.0")
Review Comment:
nit: what is the behavior of the controller default? perhaps you should elaborate on that here so it is clear to users
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -92,17 +92,21 @@ public Map<String, RebalancePreCheckerResult> check(PreCheckContext preCheckCont
// Check whether minimizeDataMovement is set in TableConfig
preCheckResult.put(IS_MINIMIZE_DATA_MOVEMENT,
checkIsMinimizeDataMovement(tableConfig, rebalanceConfig, tableRebalanceLogger));
+ // Determine the disk utilization threshold to use - either from rebalance config override or default
+ double diskUtilizationThreshold = rebalanceConfig.getDiskUtilizationThreshold() >= 0.0
+ ? rebalanceConfig.getDiskUtilizationThreshold() : _defaultDiskUtilizationThreshold;
+ // clip the disk utilization threshold to [0.0, 1.0]
+ diskUtilizationThreshold = Math.min(1.0, diskUtilizationThreshold);
Review Comment:
let's log a warning if we clip so users are aware in case they wonder why the value they provided changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #16144:
URL: https://github.com/apache/pinot/pull/16144#discussion_r2155902105
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/RebalanceServer/RebalanceServerOptions.ts:
##########
@@ -174,5 +174,15 @@ export const rebalanceServerOptions: RebalanceServerOption[] = [
"isAdvancedConfig": true,
"isStatsGatheringConfig": false,
"markWithWarningIcon": false
+ },
+ {
+ "name": "diskUtilizationThreshold",
+ "defaultValue": -1.0,
+ "type": "DOUBLE",
+ "label": "Disk Utilization Threshold",
+ "description": "Override disk utilization threshold used in pre-check (0.0 to 1.0, e.g., 0.85 for 85%). If not provided (or -1.0), uses the controller's default threshold",
+ "isAdvancedConfig": true,
Review Comment:
We should add the `step`, `maxValue` / `minValue` config here and use from here
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/RebalanceServer/RebalanceServerConfigurationOptions/RebalanceServerConfigurationOptionDouble.tsx:
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {Box, FormControl, TextField, Typography} from "@material-ui/core";
+import React, {useState} from "react";
+import {RebalanceServerOption} from "../RebalanceServerOptions";
+import {
+ RebalanceServerConfigurationOptionLabel
+} from "./RebalanceServerConfigurationOptionLabel/RebalanceServerConfigurationOptionLabel";
+import Utils from "../../../../../utils/Utils";
+
+type RebalanceServerConfigurationOptionDoubleProps = {
+ option: RebalanceServerOption;
+ handleConfigChange: (config: { [key: string]: string | number | boolean }) => void;
+ rebalanceConfig: { [optionName: string]: string | boolean | number };
+}
+export const RebalanceServerConfigurationOptionDouble = (
+ { option, handleConfigChange, rebalanceConfig }: RebalanceServerConfigurationOptionDoubleProps
+) => {
+ const [value, setValue] = useState<number>(
+ Utils.getRebalanceConfigValue(rebalanceConfig, option) as number
+ );
+ return (
+ <Box display='flex' flexDirection='column'>
+ <FormControl fullWidth>
+ <RebalanceServerConfigurationOptionLabel option={option} />
+ <TextField
+ variant='outlined'
+ fullWidth
+ style={{ width: '100%' }}
+ size='small'
+ id={`rebalance-server-double-input-${option.name}`}
+ type='number'
+ inputProps={{
+ step: 0.01,
+ min: -1,
+ max: 1
+ }}
Review Comment:
We shouldn't be making this a part of the component and instead passing this from the parent, because there might be future use cases where min and max change and also step
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal merged PR #16144:
URL: https://github.com/apache/pinot/pull/16144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #16144:
URL: https://github.com/apache/pinot/pull/16144#discussion_r2159671819
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/RebalanceServer/RebalanceServerOptions.ts:
##########
@@ -174,5 +174,15 @@ export const rebalanceServerOptions: RebalanceServerOption[] = [
"isAdvancedConfig": true,
"isStatsGatheringConfig": false,
"markWithWarningIcon": false
+ },
+ {
+ "name": "diskUtilizationThreshold",
+ "defaultValue": -1.0,
+ "type": "DOUBLE",
+ "label": "Disk Utilization Threshold",
+ "description": "Override disk utilization threshold used in pre-check (0.0 to 1.0, e.g., 0.85 for 85%). If not provided (or -1.0), uses the controller's default threshold",
+ "isAdvancedConfig": true,
+ "isStatsGatheringConfig": false,
Review Comment:
It looks like it doesn't render options with `isStatsGatheringConfig=true` afaik. The only option with this flag is `preChecks`.
@himanish-star should we set this to true or not?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #16144:
URL: https://github.com/apache/pinot/pull/16144#discussion_r2155901185
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/RebalanceServer/RebalanceServerConfigurationOptions/RebalanceServerConfigurationOptionDouble.tsx:
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import {Box, FormControl, TextField, Typography} from "@material-ui/core";
+import React, {useState} from "react";
+import {RebalanceServerOption} from "../RebalanceServerOptions";
+import {
+ RebalanceServerConfigurationOptionLabel
+} from "./RebalanceServerConfigurationOptionLabel/RebalanceServerConfigurationOptionLabel";
+import Utils from "../../../../../utils/Utils";
+
+type RebalanceServerConfigurationOptionDoubleProps = {
+ option: RebalanceServerOption;
+ handleConfigChange: (config: { [key: string]: string | number | boolean }) => void;
+ rebalanceConfig: { [optionName: string]: string | boolean | number };
+}
+export const RebalanceServerConfigurationOptionDouble = (
+ { option, handleConfigChange, rebalanceConfig }: RebalanceServerConfigurationOptionDoubleProps
+) => {
+ const [value, setValue] = useState<number>(
+ Utils.getRebalanceConfigValue(rebalanceConfig, option) as number
+ );
+ return (
+ <Box display='flex' flexDirection='column'>
+ <FormControl fullWidth>
+ <RebalanceServerConfigurationOptionLabel option={option} />
+ <TextField
+ variant='outlined'
+ fullWidth
+ style={{ width: '100%' }}
+ size='small'
+ id={`rebalance-server-double-input-${option.name}`}
+ type='number'
+ inputProps={{
+ step: 0.01,
+ min: -1,
+ max: 1
+ }}
Review Comment:
We shouldn't be making this a part of the component and instead passing this from the config, because there might be future use cases where min and max change and also step
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16144:
URL: https://github.com/apache/pinot/pull/16144#issuecomment-2986142341
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16144?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `90.90909%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 33.32%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`ea68d1c`)](https://app.codecov.io/gh/apache/pinot/commit/ea68d1c9394bdca0ef5baecd06d8641e14de3ea6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 297 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16144?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...lix/core/rebalance/DefaultRebalancePreChecker.java](https://app.codecov.io/gh/apache/pinot/pull/16144?src=pr&el=tree&filepath=pinot-controller%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcontroller%2Fhelix%2Fcore%2Frebalance%2FDefaultRebalancePreChecker.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9EZWZhdWx0UmViYWxhbmNlUHJlQ2hlY2tlci5qYXZh) | 80.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16144?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (ea68d1c). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (ea68d1c) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|0|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|1|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16144 +/- ##
=============================================
- Coverage 62.90% 33.32% -29.58%
+ Complexity 1386 659 -727
=============================================
Files 2867 2951 +84
Lines 163354 169780 +6426
Branches 24952 25959 +1007
=============================================
- Hits 102755 56587 -46168
- Misses 52847 108650 +55803
+ Partials 7752 4543 -3209
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.32% <90.90%> (-29.50%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.32% <90.90%> (-29.58%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.32% <90.90%> (-29.57%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16144/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.32% <90.90%> (-0.25%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16144?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on PR #16144:
URL: https://github.com/apache/pinot/pull/16144#issuecomment-2992994335
@somandal
> Should we fail the rebalance if a number outside of 0.0 and 1.0 is provided? or consider it as disabled if set to some invalid number?
Let's not fail it as it's minor (the threshold is only used in the number comparison). I now cap it to [0.0, 1.0] in the pre-checker, which should be enough.
@himanish-star
I updated the component to have `valueStep` `valueMin` and `valueMax`, please let me know if this is okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16144:
URL: https://github.com/apache/pinot/pull/16144#discussion_r2155539059
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java:
##########
@@ -673,6 +673,9 @@ public RebalanceResult rebalance(
@QueryParam("retryInitialDelayInMs") long retryInitialDelayInMs,
@ApiParam(value = "Whether to update segment target tier as part of the rebalance") @DefaultValue("false")
@QueryParam("updateTargetTier") boolean updateTargetTier,
+ @ApiParam(value = "Disk utilization threshold override (0.0 to 1.0, e.g., 0.85 for 85%). "
Review Comment:
should you call out this is only used for pre-checks?
##########
pinot-controller/src/main/resources/app/components/Homepage/Operations/RebalanceServer/RebalanceServerOptions.ts:
##########
@@ -174,5 +174,15 @@ export const rebalanceServerOptions: RebalanceServerOption[] = [
"isAdvancedConfig": true,
"isStatsGatheringConfig": false,
"markWithWarningIcon": false
+ },
+ {
+ "name": "diskUtilizationThreshold",
+ "defaultValue": -1.0,
+ "type": "DOUBLE",
+ "label": "Disk Utilization Threshold",
+ "description": "Override disk utilization threshold (0.0 to 1.0, e.g., 0.85 for 85%). If not provided (or -1.0), uses the controller's default threshold",
Review Comment:
let's call out in the description that this is for pre-checks only
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalanceConfig.java:
##########
@@ -136,6 +136,12 @@ public class RebalanceConfig {
@ApiModelProperty(example = "300000")
private long _retryInitialDelayInMs = 300000L;
+ // Disk utilization threshold override. If set, this will override the default disk utilization threshold
+ // configured at the controller level. Value should be between 0.0 and 1.0 (e.g., 0.85 for 85%)
+ @JsonProperty("diskUtilizationThreshold")
+ @ApiModelProperty(example = "0.85")
+ private double _diskUtilizationThreshold = -1.0; // -1.0 means not set, use default
Review Comment:
nit: reword to "-1.0 means override is disabled, use controller config default"
Perhaps add this to the comments right above the property instead of here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new parameter in Table Rebalance API: Disk Utilization Pre-check Threshold Override [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #16144:
URL: https://github.com/apache/pinot/pull/16144#issuecomment-2986068803
> When the value is 15 (no check on this value, though)
Should we fail the rebalance if a number outside of 0.0 and 1.0 is provided? or consider it as disabled if set to some invalid number?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org