Skip to content
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

Merged
merged 20 commits into from
Jun 22, 2023

Conversation

adarshsanjeev
Copy link
Contributor

@adarshsanjeev adarshsanjeev commented Jun 15, 2023

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.

{
    "interval": "2019-08-25T12:00:00.000Z/2019-08-26T00:00:00.000Z",
    "type": "loadByInterval",
    "allowEmptyTieredReplicants": true
}

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.
Screenshot 2023-06-20 at 8 39 57 AM


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Contributor

@kfaraz kfaraz left a 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?

@adarshsanjeev
Copy link
Contributor Author

I think Option 1 has the potential to be confusing. For example

  {
    "type": "loadByInterval",
    "tieredReplicants": {},
    "interval": "2010-01-01/2020-01-01"
  }

and

  {
    "type": "loadByInterval",
    "interval": "2010-01-01/2020-01-01"
  }

would result in one loading the segments on historicals and one would not, which would end up being difficult to understand moving forward.

@cryptoe
Copy link
Contributor

cryptoe commented Jun 16, 2023

We could also include a new flag called allowNullTieredReplicants which is defaulted to true and does not change the current behavior ie we set the default tier rules in case NullTieredReplicants is null.

If users set this to false the new behavior is enabled. The web-console starts setting this new flag for every request. That would solve the backward compatibility piece.
Its Option 1 with a new flag.

cc @kfaraz @adarshsanjeev

Copy link
Contributor

@cryptoe cryptoe left a 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.

Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes required.

Copy link
Contributor

@cryptoe cryptoe left a 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.

Copy link
Contributor

@kfaraz kfaraz left a 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 !

Copy link
Contributor

@abhishekrb19 abhishekrb19 left a 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?

docs/operations/rule-configuration.md Outdated Show resolved Hide resolved
docs/operations/rule-configuration.md Outdated Show resolved Hide resolved
@kfaraz kfaraz added the Needs web console change Backend API changes that would benefit from frontend support in the web console label Jun 22, 2023
@kfaraz kfaraz merged commit 90b8f85 into apache:master Jun 22, 2023
72 checks passed
abhishekagarwal87 pushed a commit that referenced this pull request Jul 17, 2023
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
AmatyaAvadhanula pushed a commit to AmatyaAvadhanula/druid that referenced this pull request Jul 17, 2023
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
abhishekagarwal87 pushed a commit that referenced this pull request Jul 17, 2023
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>
@abhishekagarwal87 abhishekagarwal87 added this to the 27.0 milestone Jul 19, 2023
sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023
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.
sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023
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
@vogievetsky vogievetsky removed the Needs web console change Backend API changes that would benefit from frontend support in the web console label Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants