Hard limit priority weight total by int32 value #38125
Conversation
Taragolis
added
the
full tests needed
|
Nice! |
Taragolis
changed the title
| @@ -24,6 +24,11 @@ Priority Weights | |||
| bumped to any integer. Moreover, each task has a true ``priority_weight`` that is calculated based on its | |||
| ``weight_rule`` which defines the weighting method used for the effective total priority weight of the task. | |||
|
|
|||
| .. versionadded:: 2.9.0 | |||
|
|
|||
| Total priority weight should be in range between **-2,147,483,648** and **2,147,483,647**. | |||
There was a problem hiding this comment.
Do we support negative value for weight?
There was a problem hiding this comment.
The value doesn’t have any semantic meanings, things are simply ordered by it in the scheduler. A negative value works as well as positive, or zero.
There was a problem hiding this comment.
Thinking about this though, I wonder if we should just change the database field to a float instead. We don’t really care about the precise value here, and a float can be ordered as well as an int.
There was a problem hiding this comment.
That's an interesting thought. might be indeed simple to implement - just migration - and does not require any code changes
There was a problem hiding this comment.
Yeah this is also applicable. It still required to change a code: Models + Migration
Just one nit this change might invoke internally recreate Task Instance table (delete old records, create new records). In user perspective it might required some time on huge TI table.
There was a problem hiding this comment.
With current implementation it is hard to overflow even int32 (limits for MySQL and Postgres) priority of task + sum priorities of upstream/downstream tasks.
But it might changed in case of #36029, some custom user defined WeightRules could easily overflow any values in case of progressive and exponential progressive implementations.
There was a problem hiding this comment.
Anyway, I will try to this approach, and we could decide which one is better suits for current and potential future implementations
There was a problem hiding this comment.
In user perspective it might required some time on huge TI table.
We recommand to clean old records periodicly and before upgrade.
There was a problem hiding this comment.
Even if we recommended it doesn't mean that it does not impact someone who even uses this recommendation.
This one only changes validation in one place and someone who does not use priorities probably does not even notice about this change but it introduces hard limits which already exist, but rather that crash scheduler it changed values to suitable.
The other one is required to change type, find which type better suits, write migrations and change types from int to float.
|
Closed, it should be implemented in top of the #38222 |
Another solution for prevent overflow total task priority. Just limit it by int32 value and use it
Another one: #37990
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.