Added condition to check if it is a scheduled save or rerun #43453
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
Can you please add a unit test covering it? |
We will, but first we want to know if you (== anyone responsible for dbt Cloud Operator) are fine with the approach |
That was the part of first pass of review. At this stage assesment from me is that the change lacks unit test - so it won't be generally accepted anyway.
Maybe. I have no idea, I am not a dbt expert. Possibly someone else who is will take a look. But a lot of people won't even look if there is no unit test, we generally don't accept PRs where unit tests work before and after the change - because it means that the change is not testable and prone to regression. Also unit tests help to guide reviewer in understanding what the change does - unit tests shows the scenarios in which the change gets used. Often unit test explain the reason and circumstances of the change better than words, and as such they are often used - especially when changes are small - as the review guideline. Also it shows that the author knows what they are doing since they are able to reproduce the situation they are talking about in unit tests. |
|
BTW. There is no-one "responsible" - this is not how open-source work. There are > 3000 mostly volunteers - who similarl y as you - are both contributing and reviewing the code in their free time away from their day jobs and families, so the best course of action to get your PR merged, is to follow reviewer's comments and asks, and ping (in general not individual people) that your PR needs review. |
We know it won't be accepted - the point is to get approval to finalize the PR.
I don't see any value in writing unit tests before there's confirmation the approach is fine, otherwise it's just a waste of time of contributors. You start with design, not with coding (that's why the confirmation should happen first in the issue anyway, the PR was opened because you asked for it), and the reasoning is described in both the issue and MR description. It's more than enough to provide a feedback if it makes sense conceptually for someone familiar with given provider, especially in a situation when a few potential solutions were proposed.
But there has to be someone responsible for the provider, who knows its specifics and who takes the final decision - I guess you don't merge PRs related to providers you don't know just because they seem to be documented and tested, you have to understand the impact. |
Yes. You trade the time of contributors with time of maintainers. And no. It's not a general "truth" that things start from design. There is an idea of "emerging design" that comes from discussion on a code implemented and tests showing how it work. There is even "test driven development" so statement suggesting that there is the only way of doing things starting with design is totally unfounded. And here we prefer to see the code and tests to be able to assess the impact - that saves precious time of maitainers who (I am sure you can attempt to empathise wiht them) sometimes review 10s of PRs a day and do it in their free, voluntary time. So the best thing you can do is to follow the way we do it here. If we follow things each of 3000+ contributors think is "better" , that would make it impossible to manage the flow of incoming contributions. So if you would like to join 3000 contributors, I think it makes sense that you adapt to the way how they work, not the other way round. Or at least that seems more reasonable from common sense point of view.
No. This is how things work in corporate-driven projects. This is not how things work in an open-source projects governed by the Apache Software Foundation. There is no-one responsible for this particular piece. You can see the history of contributors by following git history of the files you modified And see if there are changes relevant to your and ping those contributors if you think their opinion is relevant, but list of all contributors is all that you can see this way. We merge changes if the author (and reviewers who might or might not be maintainers) convince us that they tested the change and review looks good. Also when we release such a change, that is merged the authors are asked to check release candidates to confirm that their change worked as expected and that they tested it - see for example here #43615. And we can always revert or fix it when you find it's not during your testing, and produce rc2 etc. So ultimately it's actually your responsibility as an author to test and "convince" maintainers that the change is tested enough and to test it when release candidate is sent for voting . This is why also we ask you to add unit tests, to get more confidence that you understand the process and you are going to follow it - including testing the release. We treat contributors very seriously here. |
We don't trade the time of contributors with time of maintainers - it's beneficial for both sides to agree on the direction before you go deep into code changes. Neither "emerging design" nor TDD means you start without the expected goal. Although you want to say you first write tests, then you write logic, and only after that you start defining the outcome in issues / user stories, but that would be, well, very unique approach.
Yes, we will follow the "PR requirements", but that's not the point. In that case we can do the full implementation and be blocked at the very last stage because of something aimed to be discussed at the beginning, or we can implement something very opinionated which will be accepted just because "code and tests look fine". Neither outcome is good.
We are back to the beginning again. The whole discussion is not about "convincing" anyone if the implementation is correct, that part is quite obvious, but about the common agreement which approach makes sense. We would change the logic, someone else would say they prefer the another one and create a new PR, and we will go back-and-forth indefinitely? If you say there's no one within Airflow contributors to make such decision (which I really don't get - someone has to be a reviewer and someone has to accept the PR anyway - not only, I hope, by reviewing the quality of code - so such person could provide a feedback already now as well), then we can just reach out to dbt Labs folks to find the "decision maker" there. With a hope that their agreement would be a sufficient argument for reviewers to accept the agreed solution (once implementation is finalised, of course). |
|
Sorry, I have no time to lose on that discussion, but I think if you want to contribute, I suggest you to follow reviewer's request rather than argue with them. This is not the best way to contribute. And if you are not able to understand and adjust, maybe simple open source contribution is not for you. |
|
And yes. if you want to reach out to DBT maintainers - feel free. As I said we treat contributors seriously, if you need |
Of course you do - respecting their time and themselves in the way presented above is the best proof of this 😉 Sorry for wasting your time by raising concerns that you are unable to answer without ad personam. But in fact, it is a valuable feedback that we can significantly change the logic of the operator and no one will even wonder about the rationale behind it - as long as there are tests. We will definitely raise that issue during discussion with |
Please do. Maybe they will contribute back - including the tests. That's all I am asking for - to test providers and provide unit test for them. If your discussion with I also have not noticed any |
krzysztof-kubis
requested review from
eladkal,
potiuk,
ashb,
jedcunningham,
mobuchowski,
o-nikolas,
mik-laj,
jscheffl,
hussein-awala,
dstandish and
uranusjr
as code owners
krzysztof-kubis
requested review from
bbovenzi,
pierrejeambrun,
ryanahamilton,
ephraimbuddy,
bolkedebruin and
XD-DENG
as code owners
pierrejeambrun
changed the title
|
Hello guys, @krzysztof-kubis @jaklan do you plan on continuing your work on that PR, just to know what do to with this one? This will most likely help many other users facing the same problem, I think it's worth pursuing. I noticed the branch is in a weird state, most likely due to a bad rebase I assume. Best regards, |
Solved! |
krzysztof-kubis
force-pushed
the
43347-problem-with-retry_from_failure-flag-in-DbtCloudRunJobOperator
branch
from
31d3fda
to
d988fa2
Compare
There was a problem hiding this comment.
LGTM - the tests now explain what is the expectation . Thanks for adding them! @pierrejeambrun Are you also OK with it?
|
Next week we have a call with (although it's also not perfect because Airflow task could be retried not because of dbt Cloud job failure) |
Nothing wrong with incremental changes. The dbt folks were, at least peripherally, involved in the initial creation of this provider. Continued collaboration is definitely welcomed. This provider has come a long way since it was created with contributions from folks like you @krzysztof-kubis, so thank you! |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
|
🎉 Congrats @krzysztof-kubis! Keep the contributions coming! |
As you wish, just to note it's already a breaking change - with the second approach we could avoid it by either:
|
Indeed |
|
Despite some grumpy maintainers :). But you know guys, I am Polish as both of you @jaklan @krzysztof-kubis , so I had to complain. Sorry if you were a bit too much put-off inititally, and if I was a bit too harsh, but this OSS world is a bit different than traditional and the "top-bottom" thing does not work here - much more responsibility is in the hands of contributors. |
@potiuk Sure, we get this point, but that's exactly the reason why we had doubts about the best way to move forward. It's just a single-line change, but - breaking change. And the fact we expect the Operator to behave differently doesn't mean it's the same for other users, maybe for most of them the existing behaviour was actually fine? That's why we wanted to ensure first there was a common agreement to change the logic, but if there is no direct ownership of the Operator - then I believe we should be even more cautious about introducing breaking changes, which could be avoided for example in one of the above ways:
|
Yeah - and that's where seeing test helps and - as you see there were people who looked at it and said "yes, looks fine, let's merge it'. We have 4000+ operators in Airflow. There is no way we can be authoritative in all of them as maintainers. But you already said you feel that you would like to discuss it with This is how it works here - committer (also known as maintainer) is not a person who is "responsible" for some part of the code - this is a person who is committed and engaged in the project, but besides of being able to approve and merge the code that "looks good" and that author is confident about fixing some problems and confirming they tested it, they have no "authority" / "responsibilty" to make decisions over specific part of the code alone. Yes. I know it's surprising that it works, but it does. Because authors like you take more responsibility for their own decisions when their code is merged and their name is attached to "I made that change and vouch for it". The code looks good, we cannot see any big issue and we trust you care and made good decision. Worst thing that can happen, people will downgrade the provider and we will have to revert that change if it will turn it has more problems that we foresaw by reviewing them as maintainers, no big harm done - but we trust you will test it with release candidate as well and confirm it's working. |
…3453) * Aadded condition to check if it is a scheduled save or rerun * Fix key name in context of task * I added unit tests to the condition to check if it is a scheduled save or rerun --------- Co-authored-by: RBAMOUSER\kubisk1 <[email protected]> Co-authored-by: krzysztof-kubis <[email protected]> Co-authored-by: krzysztof-kubis <[email protected]/>
|
Hey! I'm the DX advocate for the orchestration team at dbt Labs. I think this approach (only issue a rerun command if it's an Airflow retry) makes sense. It aligns with the behaviour in the run results UI for dbt Cloud, where the only way you can trigger a rerun is to have a job which has partially failed. So it should match up with folks' mental models pretty nicely. As for the tradeoffs between the possible approaches, I think it would have been useful to flag that it was a breaking change when the PR was opened, as opposed to after the merge had happened. The initial issue enumerated the options but didn't explicitly call out the tradeoffs associated with each one. (To be clear, I do think you made the right choice in not adding a new option/parameter, since the change aligns with the mental model of the feature in dbt Cloud so should be uncontroversial.) Thanks for your contribution to the operator! |
|
@joellabes thanks for the input! The most confusing part here was whether we should take "UI mental model" or "API mental model":
So one could argue the expected behaviour of the operator option is just to switch the endpoint which is used. We were in favor of the first approach, but we didn't want to take that decision by ourselves - now it's confirmed it's aligned with your vision as well 😉 Having said that, I wonder how we can make it more robust. With the current implementation, Airflow task can fail for whatever reason not related to dbt Cloud job (e.g. queue timeout) and be retried, which could trigger a significantly different behaviour if the previous run (e.g. from the previous day) in dbt Cloud didn't succeed - and that could lead to unexpected results. Quick idea could be to check if there's any dbt Cloud run ID associated with the first Airflow task try and based on its existence / status (+
That's true, we haven't expected things to happen so fast 😄 Especially we thought that comment would simply "freeze" the MR until we have a discussion during the today meeting, but well, it didn't 🙈 |
It'll only retry if the most recent run of the job failed - if yesterday morning's run failed in dbt Cloud, and yesterday evening's run succeeded, and then this morning's run failed in Airflow, an Airflow retry will trigger a whole new run in Cloud instead of picking up from halfway through yesterday morning's Cloud run. So I think it should be ok? |
In this case - yes, but I mean a different scenario:
|
What?
I added a condition distinguishing a scheduled run from a rerun.
retry_from_failure=Trueis set, should only build models that failed in the previous run.Why?
The problem is that if there is an error in one model that can't be resolved (e.g., due to a data source issue), the flag prevented the other models from being refreshed, even in subsequent scheduled runs.
How?
{account_id}/jobs/{job_id}/run/{account_id}/jobs/{job_id}/rerun/^ 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.