potiuk commented on a change in pull request #19572:
URL: https://github.com/apache/airflow/pull/19572#discussion_r774969981
##########
File path: airflow/providers/cncf/kubernetes/CHANGELOG.rst
##########
@@ -19,6 +19,19 @@
Changelog
---------
+3.0.0
+.....
+
+Breaking changes
+~~~~~~~~~~~~~~~~
+
+* ``Simplify KubernetesPodOperator (#19572)``
+
+.. warning:: Many methods in :class:`~.KubernetesPodOperator` and
class:`~.PodLauncher` have been renamed.
+ If you have subclassed :class:`~.KubernetesPodOperator` will need to
update your subclass to reflect
+ the new structure. Additionally ``PodStatus`` enum has been renamed to
``PodPhase``.
Review comment:
I think it should not be a general approach and it should not apply to
all such breaking changes.
I certainly do not treat it as 'general' approach that should be used in
other cases. Not at all.
I only proposed it because it is the generic Kubernetes Pod Operator ans the
nature of the change which is - I think - a very special case. We should - i
think- treat it differently a bit - similarly to Python Operator, Bash Operator
- those are the operators that are basic building blocks for many workflows. In
a way - even if it separate provider, for many people KPO is part of Airflow.
Most of the users who use KPO likely use Airflow with Helm Chart and this means
that when they upgrade Airflow to the next non-breaking change they will get
the.umage with updated KPO and if they workflows will break - they will
perceive it as if Airflow introduced breaking change.
And we will not avoid that one. But what I am afraid here is that they might
not even realize it only after some time after migration. The nature of the
change where public methods of KPO were likely to be overwritten by the users
is such, that the customizes operator might continue to work - and only after
some time the users will realize that their customisations stopped working. And
this is an absolute nightmare scenario - for both us and our users. Because it
will be arleady too late for many cases. I can imagine a number of cases here -
for example someone injects custom labels or adds some behaviour to stop
sidecars (which i think might be a very common scenario as we do not provide a
good solution for that). I think explicitly failing such workflows is far
better approach. Users will find out during their test migration that they have
to modify their customisations to make them work this way - not after they
already migrated in production.
I think also we should only do that if we assess that it's likely that many
of our users were already extending the operator with various modifications if
they missed some features in the past.
I am not sure it was the case but i have a hunch it is very likely. I am
sure we can look at stack overflow/GitHub issues/slack to see if people were
asking for it and whether there were some advices that this was the right way.
If we assess it was unlikely - i am all for not using MetaClass. But if we see
that there are advices which suggest people to go that route, we should likely
protect the users from surprises.
Also - i think this is the second time we make such breaking changes in KPO.
I believe there was already a change there and I recall (but this is just
feeling that we could easily try to see from the historical issues) it caught
quite a number of surprised users as I recall and caused some Mayhem. I think
we should try to take a look at that and assess how likely it is going to occur
again - simply learning from the past.
At the end this is really an exercise of guessing and anticipating. If we
anticipate that a lot of users will be caught by surprise.
BTW I am super happy to do such historical assessment and make an inventory
of past experiences with KPO - i can do that after Xmas - just to make the
case stronger (or convince myself it is not worth it).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
