-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow empty tiered replicants map for load rules #14432
Conversation
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java
Fixed
Show fixed
Hide fixed
server/src/test/java/org/apache/druid/server/coordinator/rules/IntervalLoadRuleTest.java
Fixed
Show fixed
Hide fixed
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java
Fixed
Show fixed
Hide fixed
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java
Fixed
Show fixed
Hide fixed
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.
I think this change is backwards incompatible. There must be several clusters using null
which internally gets converted to the mapping _default_tier => 2
. You are now converting these to the mapping _default_tier => 0
in the LoadRule.run()
method. This will cause unexpected behaviour in all such clusters.
I can think of some alternatives:
Option 1 (preferable)
- Continue to treat
null
as a special and convert it to_default_tier => 2
as before - Also allow empty in the
LoadRule.run()
. It would automatically translate to mean the segment need not be loaded anywhere. - Allow
0
replicas to be specified for any tier for the same backward compatibility reasons. I don't think this affects the coordinator behaviour in any way.
The only con of this method is that null
and empty
have different meanings.
OR
Option 2 (not very user-friendly)
- Keep everything as is.
- We must specify mapping for at least one tier even it we don't want segment to be loaded anywhere.
Option 3 (reserve a special tier name)
- Similar to option 2 but use a special tier-name which can be mapped to 0 instead of an actual tier in the cluster.
@adarshsanjeev , @cryptoe , what do you think?
I think Option 1 has the potential to be confusing. For example
and
would result in one loading the segments on historicals and one would not, which would end up being difficult to understand moving forward. |
We could also include a new flag called If users set this to |
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.
Just suggested a new name.
server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java
Outdated
Show resolved
Hide resolved
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.
Minor changes required.
server/src/main/java/org/apache/druid/server/coordinator/rules/IntervalLoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Outdated
Show resolved
Hide resolved
services/src/test/java/org/apache/druid/server/router/CoordinatorRuleManagerTest.java
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/coordinator/rules/PeriodLoadRuleTest.java
Outdated
Show resolved
Hide resolved
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.
Overall lgtm!!
Will merge once @kfaraz gives his approval.
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/PeriodLoadRule.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java
Outdated
Show resolved
Hide resolved
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 for the changes, @adarshsanjeev !
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.
LGTM. Do we want to document the new parameter useDefaultTierForNull
: https://github.com/apache/druid/blob/master/docs/operations/rule-configuration.md?
server/src/main/java/org/apache/druid/server/coordinator/rules/ForeverLoadRule.java
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
Show resolved
Hide resolved
server/src/test/java/org/apache/druid/server/coordinator/rules/ForeverLoadRuleTest.java
Show resolved
Hide resolved
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API #14512 Allow empty tiered replicants map for load rules #14432 Adding Interactive API's for MSQ engine #14416 Add replication factor column to sys table #14403 Account for data format and compression in MSQ auto taskAssignment #14307 Errors take 3 #14004
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API apache#14512 Allow empty tiered replicants map for load rules apache#14432 Adding Interactive API's for MSQ engine apache#14416 Add replication factor column to sys table apache#14403 Account for data format and compression in MSQ auto taskAssignment apache#14307 Errors take 3 apache#14004
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API #14512 Allow empty tiered replicants map for load rules #14432 Adding Interactive API's for MSQ engine #14416 Add replication factor column to sys table #14403 Account for data format and compression in MSQ auto taskAssignment #14307 Errors take 3 #14004 Co-authored-by: Vadim Ogievetsky <vadim@ogievetsky.com>
Changes: - Add property `useDefaultTierForNull` for all load rules. This property determines the default value of `tieredReplicants` if it is not specified. When true, the default is `_default_tier => 2 replicas`. When false, the default is empty, i.e. no replicas on any tier. - Fix validation to allow empty replicants map, so that the segment is used but not loaded anywhere.
This PR catches the console up to all the backend changes for Druid 27 Specifically: Add page information to SqlStatementResource API apache#14512 Allow empty tiered replicants map for load rules apache#14432 Adding Interactive API's for MSQ engine apache#14416 Add replication factor column to sys table apache#14403 Account for data format and compression in MSQ auto taskAssignment apache#14307 Errors take 3 apache#14004
This PR makes some changes around the allowed tiered replicants for load rules. It allows values empty tiered replicant in load rules, which would translate to the segment being marked as used, but not loaded on any historical for example:
For backwards compatibility, a new flag "allowEmptyTieredReplicants" is also added. If the flag is false or missing, the old behaviour of converting an empty map to the default load rule is maintained. If it is true, which would be the case going forward, empty load rules are allowed.
The above load rule would not be loaded on the historical as per the new behaviour.
Exceptions thrown during validation are returned to the user.
This PR has: