You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2023/06/26 07:46:05 UTC
[GitHub] [airflow] uranusjr opened a new pull request, #32132: Fixup config var types under the scheduler section
uranusjr opened a new pull request, #32132:
URL: https://github.com/apache/airflow/pull/32132
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32132: Fixup config var types under the scheduler section
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32132:
URL: https://github.com/apache/airflow/pull/32132#issuecomment-1607593040
> @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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #32132: Fixup config var types under the scheduler section
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32132:
URL: https://github.com/apache/airflow/pull/32132#issuecomment-1609014406
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] hussein-awala commented on pull request #32132: Fixup config var types under the scheduler section
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #32132:
URL: https://github.com/apache/airflow/pull/32132#issuecomment-1607568186
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #32132: Fixup config var types under the scheduler section
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32132:
URL: https://github.com/apache/airflow/pull/32132#issuecomment-1607209690
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #32132: Fixup config var types under the scheduler section
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr merged PR #32132:
URL: https://github.com/apache/airflow/pull/32132
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org