Skip to content

Conversation

@uranusjr
Copy link
Member

I got curious when reading #32123 and decided to take a look at the old configs in scheduler. Many configs are currently typed as string, but only used as strongly-typed values in code.

Many configs are currently typed as string, but only used as
strongly-typed values in code.
@potiuk
Copy link
Member

potiuk commented Jun 26, 2023

Side comment - I think those kind of changes should be treated carefully. I do remember a case when changing type of the configuration introduced breaking change that we were struggling to fix (I think it was chaning an int into float or the other way). Not sure if there will be some effects here but I think some careful thinking here of edge scenarios and looking at the code when those changed values are used should be part of the change. There might be non-obvious side-effects.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

LGTM

@hussein-awala
Copy link
Member

@potiuk, it could be changing a float to an int as it might cause a failure if a float value is provided. However, overall, this PR seems legit to me.

@potiuk
Copy link
Member

potiuk commented Jun 26, 2023

@potiuk, it could be changing a float to an int as it might cause a failure if a float value is provided. However, overall, this PR seems legit to me.

It does. Yes.

But I think at the very least we should check if in all of the places where the configs are used we use get_<type>() and what happens there (and possibly correct if we don't).

Just this, similar things were biting us in the past, so I am trying to be cautious :)

@uranusjr
Copy link
Member Author

Definitely agree that changing types should be handled carefully. I specifically only touched values typed as string for this reason; it’s by far the safest variant.

@uranusjr uranusjr merged commit 1f22db7 into apache:main Jul 3, 2023
@uranusjr uranusjr deleted the job_heartbeat_sec_float branch July 3, 2023 07:57
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Jul 6, 2023
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants