[hotfix][Optimizer] Guard invalid fragment ratio and target quota - #4324
[hotfix][Optimizer] Guard invalid fragment ratio and target quota#4324czy006 wants to merge 1 commit into
Conversation
4749baf to
a218435
Compare
| return targetSize / fragmentRatio; | ||
| // A non-positive fragment ratio (misconfigured property or legacy persisted value) means | ||
| // "no fragmenting": fall back to the target size instead of dividing by zero in planners. | ||
| return fragmentRatio <= 0 ? targetSize : targetSize / fragmentRatio; |
There was a problem hiding this comment.
the default target-size is 134217728 which is too large
There was a problem hiding this comment.
Good catch. I updated the fallback to use the documented default fragment ratio of 8, so the default 128 MiB target size yields a 16 MiB fragment threshold. CommonPartitionEvaluator now calls OptimizingConfig.maxFragmentSize() instead of performing the raw division, and a planner-level regression test covers both zero and negative ratios.
a218435 to
7cd19a5
Compare
czy006
changed the title
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class OptimizingConfig { | ||
|
|
||
| public static final int DEFAULT_FRAGMENT_RATIO = 8; |
There was a problem hiding this comment.
Why define the defaut value there
There was a problem hiding this comment.
Agreed. I removed the default constant from OptimizingConfig; the configured default remains owned by TableProperties.
| public long maxFragmentSize() { | ||
| return targetSize / fragmentRatio; | ||
| int effectiveFragmentRatio = fragmentRatio > 0 ? fragmentRatio : DEFAULT_FRAGMENT_RATIO; | ||
| return targetSize / effectiveFragmentRatio; |
There was a problem hiding this comment.
| return targetSize / effectiveFragmentRatio; | |
| return targetSize / Math.max(1, fragmentRatio); |
There was a problem hiding this comment.
I think we can also add a upper limitation for fragmentRatio. it should be less or equals than targetSize
There was a problem hiding this comment.
I believe the param would be validated in setFragmentRatio instead of getter method. WDYT
There was a problem hiding this comment.
Thanks. Following the later setter-validation suggestion, the lower bound is now applied once in setFragmentRatio() instead of inside maxFragmentSize().
There was a problem hiding this comment.
Agreed and updated. setFragmentRatio() now normalizes zero and negative values to 1, and maxFragmentSize() remains a simple derived calculation. The evaluator also reuses this method.
There was a problem hiding this comment.
I have not added the upper bound in this hotfix. fragmentRatio is dimensionless while targetSize is a byte size, so this would introduce a new cross-field invariant. Also, TableConfigurations currently calls setFragmentRatio() before setTargetSize(), so the setter cannot validate against the final target size reliably. A very large positive ratio produces a zero fragment threshold but does not cause the division failure addressed here. I suggest handling an upper-bound policy separately, with defined semantics and validation after both values are available.
|
|
||
| public static final String SELF_OPTIMIZING_FRAGMENT_RATIO = "self-optimizing.fragment-ratio"; | ||
| public static final int SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT = 8; | ||
| public static final int SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT = |
There was a problem hiding this comment.
| public static final int SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT = | |
| public static final int SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT = 8; |
There was a problem hiding this comment.
Done. SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT is restored to 8 in TableProperties.
self-optimizing.fragment-ratio and self-optimizing.quota do not enforce positive values, so zero or negative values may already exist in persisted table properties. Normalize fragment ratios to at least one in OptimizingConfig.setFragmentRatio(), keep the configured default value of 8 in TableProperties, and make CommonPartitionEvaluator reuse OptimizingConfig.maxFragmentSize(). This prevents division by zero in the actual planner while leaving persisted properties unchanged. Clamp quota limits to at least one so invalid target quotas neither corrupt quota weights nor starve tables of schedulable slots. Tests: TestOptimizingConfig, TestCommonPartitionEvaluator, and TestOptimizingQueue#testQuotaSchedulePolicy.
7cd19a5 to
7dd6980
Compare
2 participants
Brief change log
self-optimizing.fragment-ratioto 1 inOptimizingConfig.setFragmentRatio(); the configured default remains 8 inTableProperties.OptimizingConfig.maxFragmentSize()inCommonPartitionEvaluatorso the production planner no longer performs a raw division by an invalid ratio.OptimizingConfig.How was this patch tested?
./mvnw test -pl amoro-common -Dtest=TestOptimizingConfig(2 tests passed)./mvnw test -pl amoro-format-iceberg -am -Dtest=TestCommonPartitionEvaluator(1 test passed)./mvnw test -pl amoro-ams -am -Dtest="TestOptimizingQueue#testQuotaSchedulePolicy"(1 test passed; 15 reactor modules succeeded)./mvnw validate(33 reactor modules succeeded)Documentation