[AIRFLOW-1332] Split logs based on try number - #2383
Conversation
aa1739f to
6dde129
Compare
|
I'm not very enthusiastic out this. I don't really see the use case for it. |
bolkedebruin
left a comment
bolkedebruin
left a comment
There was a problem hiding this comment.
The updates to attempt are not atomic. I don't understand try_number vs attempt.
No tests supplied.
There was a problem hiding this comment.
What are you doing with attempt? It is not used. The session is not committed and not closed.
In addition if you want to update the field make sure to read up on sqlalchemy's update mechanism in order not to end up with race conditions.
There was a problem hiding this comment.
Don't access the database directly. Use models.py in order to
Make sure you load the right ti.
There was a problem hiding this comment.
Just curious what could go wrong if I directly access db here?
There was a problem hiding this comment.
Try number vs attempt?
|
@bolkedebruin To give more context, please also refer to this PR: #2380. This change is to enable us to display logs for each task retry/rerun in different tabs instead of one long document. The try_number / attempt is indeed confusing. Basically, it's a monotonically increasing field that keeps track of the number of times a certain task instance gets picked up and executed. Try_number would become 0 once the task gets cleared, but attempt would not change. In the future, it can be used as part of the name of the log file to separate each attempt. |
|
Mmm still not convinced. Especially on elastic search it is just easier to use the start_date to shard on. In addition I'm very against the managing of log files this way. Log files are already a mess and should be refactored to be consistent and to be managed from one place. The docker packagers can already not package airflow correctly without jumping through hoops. Arbitrarily naming it certainly does not help here. |
|
@bolkedebruin I will send an email to airflow mailing list and let's move the discussion there :) |
|
Perfect. |
allisonwang
changed the title
6dde129 to
910f1a9
Compare
allisonwang
changed the title
6bd931d to
2d31df2
Compare
allisonwang
changed the title
a1abe58 to
bb7ee73
Compare
There was a problem hiding this comment.
Session is used below on line 385, you can reuse this new one there.
There was a problem hiding this comment.
Lines 404/405 already get the TI from the DB, and I think we can just reuse that. If you look at what refresh_from_db() is doing, it doesn't actually need the task to be parsed from the dagbag. We can probably move the logic from line 404/405 up here (the part that doesn't require parsing the DAGBag), and then we wouldn't need this additional query.
Reading the try number and then writing it back should be an atomic operation, otherwise two workers could get the same task instance and increment the try number twice (even though only one of them would actually run the task). refresh_from_db() does this with the lock_for_update param so you can take a look at that as an example if you don't end up rewriting the code to just use it in the first place.
There was a problem hiding this comment.
Let's factor the logic for getting this path out somewhere, it's used in a couple of places. That includes the isoformat() part and probably the log_base part too.
There was a problem hiding this comment.
Let's put dag at the start everywhere it appears in methods like this that you changed (it's the "highest level" param). Wait why do we need dag as a param actually, wouldn't dag just be self inside the clear function?
There was a problem hiding this comment.
What about the case where users upgrade from an old version of Airflow where max tries is not stored in the DB and try number might have been reset to 1 because of clearing via the UI?
There was a problem hiding this comment.
hmm not sure how users will be affected?
There was a problem hiding this comment.
Discussed offline, we are going to try to parse the DAG in the migration and set max_attempts based on the value in the DAG file.
There was a problem hiding this comment.
Cleaner is:
[''] * try_number
There was a problem hiding this comment.
Does this work as expected there is currently a running task that is not the first try that is writing to local disk and hasn't written to AWS yet? Seems like this would fail reading the last try number and not set remote_log_loaded to True when it should have.
There was a problem hiding this comment.
Let's break this set of asserts out into separate tests (they can share the setup steps if necessary by factoring them out). If all the asserts are clumped together the disadvantage is that the first failing assert will end the test case so the others won't be tested in that run.
There was a problem hiding this comment.
Nit: A little bit cleaner is
for remote_log, i in enumerate(remote_logs): (and then use remote_log instead of remote_logs[i] below)
There was a problem hiding this comment.
Doesn't the page need to know which try # it is serving (assuming that the active tab is highlighted in the UI somehow).
|
Is the code quality decrease in the landscape results a false positive? |
379d74a to
0af21a0
Compare
0af21a0 to
1e90062
Compare
There was a problem hiding this comment.
I still feel like these exceptions are too broad. Can we use if conditions instead? e.g. if dag == None?
There was a problem hiding this comment.
get_task will throw AirflowException when task_id is not found so I guess catching all exceptions at once is cleaner. We could case on the conditions but all of them will use the same way to handle those conditions as line 127.
There was a problem hiding this comment.
You can use the methods that get_task uses internally instead to avoid having to deal with exceptions:
if dag_id not in dagbag.dags:
if not dag.has_task(task_id):
Might be good to factor these into a new method task.exists if you think it makes sense.
There was a problem hiding this comment.
Let's break these asserts into individual tests.
There was a problem hiding this comment.
Let's add proper pydoc even though some of the other functions in this class do not have it.
657dfeb to
0b75dba
Compare
Codecov Report
@@ Coverage Diff @@
## master #2383 +/- ##
==========================================
- Coverage 69.4% 69.34% -0.06%
==========================================
Files 146 146
Lines 11289 11331 +42
==========================================
+ Hits 7835 7858 +23
- Misses 3454 3473 +19
Continue to review full report at Codecov.
|
b690bc0 to
cc63759
Compare
There was a problem hiding this comment.
Couldn't dag be None (otherwise why check "if not dag" above)? If so dag.get_task will fail. Would be good to write a test for this too.
There was a problem hiding this comment.
if dag is None it will fall into the first if branch? Will add more tests here.
There was a problem hiding this comment.
Nit: typo and also "for next" should be "for the next"
There was a problem hiding this comment.
Nit: s/total_try/max_tries (or at least total_tries)
There was a problem hiding this comment.
Would be good to name this something a bit more descriptive:
def log_exists(self, log_location)
There was a problem hiding this comment.
Would be good to name this something a bit more descriptive:
def log_exists(self, log_location)
There was a problem hiding this comment.
:return: True if location exists else False
There was a problem hiding this comment.
Would be good to link to something here (if we don't have a design doc skeleton yet then we can just use the refactor PR with Bolke).
There was a problem hiding this comment.
I think the logs should be in s3 here at this point of the code though right? Falling back to non-local logs might not be great, i.e. if logs should have come from a remote location and the logs don't exist there we shouldn't hide that error by showing the local logs.
There was a problem hiding this comment.
Exactly so I removed the surface_error flag and changed return_error to True when reading the remote logs.
cc63759 to
d121f55
Compare
d121f55 to
969f416
Compare
| settings.configure_vars() | ||
| settings.configure_orm() | ||
|
|
||
| if not args.pickle and not dag: |
There was a problem hiding this comment.
I think we should use is not None for these
There was a problem hiding this comment.
@saguziel Just wondering why checking is not None is better? Isn't it redundant in this case?
|
|
||
| if not args.pickle and not dag: | ||
| dag = get_dag(args) | ||
| elif not dag: |
d0a2548 to
7cc8a53
Compare
7cc8a53 to
be921c0
Compare
|
LGTM |
|
def _get_log(self, ti, log_filename): |
|
@aoen I'm a bit surprised why this was merged. I my opinion the refactor Allison is doing should have been the one going in and it will soon. This one just creates new legacy and should not arrive in any release. What was the rush? If it was really needed you could have just used it in a local build? Moreover it does not keep to the commit guidelines. With such a big change that really isn't nice. |
|
@bolkedebruin I mentioned in the other thread (logging refactor) that I would be merging this one, and didn't get any -1s so I went ahead:
With respect to the urgency, while there is definitely risk that we will not be able to complete the logging in time, and certainly not in time to test/merge/release Allison's change internally at Airbnb, I still think that saner semantics/log file separation is worth it. If the choice was between merging neither PR or just this one, I believe Airflow is better off with this one merged. By not following the commit guidelines do you mean the missing - between Airflow and the jira number? |
|
Ok I don't know if I agree with your assessment Airflow is better off this way. I wouldn't be very happy to have this in a release. But given the progress Allison is making with the other PR, one that I am willing to test, lets agree to disagree and help Allison as much as we can. Concerning this PR I though it existed of two commits, but you probably squashed them. The '-' is minor. The description isn't really what/why, but more concerning is the lack of a documentation update as it does change things operationally. This can catch people by surprise as has happened with logging before. |
I am happy to start a vote or discuss further with you via video chat if you feel it is important, please let me know. I believe pretty strongly in this change, but we should keep things democratic, and can revert the change based on the vote result. FWIW I did run this by Max early on, and he agreed with the change. In addition we created a thread in the mailing list to discuss these changes and hear people's concerns (though albeit not the slightly more complex logic that would be required on the logging side) since it was a decently-sized changed to the core.
I think we should follow 1 commit to 1 PR as much as possible on master to keep history linear and sane (and so that the JIRA tags will be unique and match correctly). I also think that 1 commit was a reasonable granularity for this change. The reason I asked Allison to create a separate commit instead of force pushing was so that it would be easy for me to safely review the changes from the 1st commit to the 2nd commit (really the 2nd commit was just bug fixes on the first one).
This is definitely true, I was operating on the precedent of previous logging changes not being communicated, but I agree this is not a good excuse. I have asked Allison to create a follow-up PR to add some additional details to the log section of the configuration.rst document. If there are additional kinds of communication you think are warranted please let me know. |
|
@aoen No need to vote. Your judgement is as good as mine. I'm not happy with it, but the right fix, imho, is in progress. Let's work together on getting that one in. On the PR squashing: I overlooked your squash that is what I meant to say. So the 'merge' was fine. Just some details with the PR itself that weren't as nice. |
5 participants
Dear Airflow maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
This PR splits logs based on try number and add tabs to display different task instance tries.
Note this PR is a temporary change for separating task attempts. The code in this PR will be refactored in the future. Please refer to #2464 for Airflow logging abstractions redesign.
Tests
Commits
@aoen @saguziel