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

Make the default TI pool slots '1' #8153

Merged
merged 1 commit into from Apr 5, 2020
Merged

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Apr 5, 2020

This is caused by the new add pool_slots field to task_instance
database migration, which adds a new pool_slots database column,
which is nullable (hence it is filled with NULLs after the migration).

This Bug was introduced in #7160 and is a blocker for 1.10.10


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

This is caused by the new `add pool_slots field to task_instance`
database migration, which adds a new `pool_slots` database column,
which is nullable (hence it is filled with NULLs after the migration).
@kaxil kaxil added this to the Airflow 1.10.10 milestone Apr 5, 2020
@kaxil kaxil requested a review from ashb April 5, 2020 02:51
@kaxil
Copy link
Member Author

kaxil commented Apr 5, 2020

I have tested this manually on my machine. I installed 1.10.9 ran a few examples DAGs, upgraded to 1.10.10rc2, Visited old Task Instance details of old tasks and was able to see the error, also could see the error in Scheduler if the TIs were still queued before I upgraded to 1.10.10rc2 and applied migrations. I applied the fix in the PR and ran the Webserver and scheduler again and both ran fine and I was able to see Task Instance details.

Note: I am not updating all the Null values in TI table for old records as this table can be pretty huge which was also the intention in the original PR: #7160 to keep old behaviour and let the column be Nullable.

@codecov-io
Copy link

codecov-io commented Apr 5, 2020

Codecov Report

Merging #8153 into master will decrease coverage by 5.83%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8153      +/-   ##
==========================================
- Coverage   66.39%   60.55%   -5.84%     
==========================================
  Files         935      935              
  Lines       45170    45170              
==========================================
- Hits        29990    27353    -2637     
- Misses      15180    17817    +2637     
Impacted Files Coverage Δ
airflow/models/taskinstance.py 94.70% <100.00%> (+31.88%) ⬆️
airflow/providers/exasol/operators/exasol.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/amazon/aws/hooks/kinesis.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/apache/livy/sensors/livy.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/amazon/aws/sensors/redshift.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/mysql/operators/s3_to_mysql.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/postgres/operators/postgres.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/microsoft/azure/operators/adx.py 0.00% <0.00%> (-100.00%) ⬇️
...irflow/providers/amazon/aws/hooks/batch_waiters.py 0.00% <0.00%> (-100.00%) ⬇️
...ow/providers/amazon/aws/sensors/cloud_formation.py 0.00% <0.00%> (-100.00%) ⬇️
... and 803 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68d1714...5576f71. Read the comment docs.

Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

Thanks @kaxil , quick response, code looking good. One more thing I am sure. Is it the new TI object column pool_slots with default value 1?

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Is this the right fix, or is changing the migration to have a default of 1 better?

@kaxil
Copy link
Member Author

kaxil commented Apr 5, 2020

Thanks @kaxil , quick response, code looking good. One more thing I am sure. Is it the new TI object column pool_slots with default value 1?

Yes

@kaxil
Copy link
Member Author

kaxil commented Apr 5, 2020

Is this the right fix, or is changing the migration to have a default of 1 better?

@ashb I didn't update the migration code as the TI table can be large for many users (which the original PR intended #7160 ). I was thinking we could do that as part of Airflow 2.0 to update NULLs to 1 and make the column not nullable. WDYT?

@ashb
Copy link
Member

ashb commented Apr 5, 2020

Is this the right fix, or is changing the migration to have a default of 1 better?

@ashb I didn't update the migration code as the TI table can be large for many users (which the original PR intended #7160 ). I was thinking we could do that as part of Airflow 2.0 to update NULLs to 1 and make the column not nullable. WDYT?

Oh yeah, adding a new column with non null default would be a table-locking migration, good point.

@kaxil kaxil merged commit 323c1d1 into apache:master Apr 5, 2020
@kaxil kaxil deleted the pool-slot-default branch April 5, 2020 12:37
kaxil added a commit that referenced this pull request Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants