[abdulbasitds]

Original PR: #4068 (Author: @oelesinsc24)

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-XXX
  • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
  • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
  • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

• Here are some details about my PR, including screenshots of any UI changes:

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

[abdulbasitds]

I wanted to work on Airflow Glue sensor and operator that wasnt ther on Airflow itself, So i s=found it on the fork of airflow (https://github.com/OElesin/incubator-airflow) and tried the glue sensor/hook/operator and they weren't working properly so i have fixed and have submiited this pull request(#6007)

[dazza-codes]

tag

• [AIRFLOW-2310]: Add AWS Glue Job Compatibility to Airflow #4068 as the source for this PR
• was wondering whether @abdulbasitds could be added as contributor on the original repo/fork to keep all the review on [AIRFLOW-2310]: Add AWS Glue Job Compatibility to Airflow #4068 alive?

[OmerJog]

@mik-laj @ashb can review? You started review on the previous stale PR

[kaxil]

Can you rebase commits and resolve conflicts, please?

I don't have much exp in AWS. but other committers can take a look @ashb

[OmerJog]

@abdulbasitds
https://help.github.com/en/github/using-git/about-git-rebase
https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing
This one is also nice
https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

you also have many examples on stackoverflow

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dazza-codes]

Sorry if this is waiting on me for some reason, but I can't promise to follow-up on review comments and would rather wait for a rebase if/when I do. Just adding this comment to mess with the stale bot.

[lwbayes]

Hi, been watching this operator for a while. Is there anything I can help to speed up this a bit? Keen to use it without having to copy and paste.

[abdulbasitds]

I was asked to "rebase commits and resolve conflicts" , Since I get time to see what is needed to be done on my side, I think it is pending on me.

If anyone can do that at their end, I think we can have this operator finally.
@letianw91 @ashb

[potiuk]

Let me guide you on this @abdulbasitds -> I think it is great to learn how to rebase in git :). I will soon put that into contributor's documentation (this is a common question - how to rebase) but I'd love if you tell me it is descriptive enough/easy to follow:

First of all - you can read about rebase workflow here: https://www.atlassian.com/git/tutorials/merging-vs-rebasing - this is an excellent article that describes all ins/outs of rebase. I recommend reading it and keeping it as reference.

The goal of rebasing your PR on top of apache/master is to "transplant" your change on top of the latest changes that are merged by others and that changes from your PR do not conflict with those changes from others.

Here is how rebase looks in practice:

1. You need to add Apache remote to your git repository. I usually add it as "apache" remote so that i can refer to it easily:
   git remote add apache git@github.com:apache/airflow.git or
   git remote add apache https://github.com/apache/airflow.git if you use https

2. You need to make sure that you have the latest master fetched from all remotes:
   git fetch --all. You want to rebase your changes on top of apache/master branch.

3. Assuming that your feature is in a branch in your repository called my-branch you can check easily what is the base commit you should rebase from
   git merge-base my-branch apache/master
   This will return you base commit which you should use to rebase your feature - for example: 5abce471e0690c6b8d06ca25685b0845c5fd270f

4. Make sure you checked out your branch locally:
   git checkout my-branch

5. Rebase:
   git rebase HASH_ID --onto apache/master
   for example:
   git rebase 5abce471e0690c6b8d06ca25685b0845c5fd270f --onto apache/master

6. If you have no conflicts - that's cool. You rebased. You can run git push --force-with-lease to push your changes to your repository. That should trigger the build in Travis if you have a Pull Request opened already.

7. While rebasing you might have conflicts. Read carefully what git tells you when it tells you about the conflicts. You need to solve the conflicts manually. This is sometimes the most difficult part and requires deliberate correcting your code looking what has changed since you developed your changes. There are various tools that can help you with that. You can use
   git mergetool (and you can configure different merge tools. Also you can use IntelliJ/PyCharm excellent merge tool. When you open project in PyCharm which has conflict you can go to VCS->Git->Resolve Conflicts and ther you have one of the most intuitive and nice merge tools I ever used. You can see there https://www.jetbrains.com/help/idea/resolving-conflicts.html.

8. After you solved conflicts simply run git rebase --continue and go either to point 6. or 7. above depending if you have more commits that cause conflicts in your PR (rebasing applies each commit from your PR one-by-one).

[potiuk]

Please let me know @abdulbasitds if it's helpful/easy to follow - I will make a PR to CONTRIBUTING.rst now but if you have any questions/comments - feel free to ask

[potiuk]

@abdulbasitds - "How to Rebase" chapter is now merged in the main CONTRIBUTING documentation of Apache Airflow:

https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#how-to-rebase-pr

[lwbayes]

Thanks @potiuk , I did a rebase on my local branch https://github.com/letianw91/incubator-airflow/commits/glue-rebase, and there is little more than resolving some conflicts in docs. It would be preferable if the author can do the rebase by himself, as the logistics will be much easier. The process can be done in around 10 minutes.

However, please do note that after the rebase, there are still some issues:

1. As commented above, there is a redundant line after last commit's changing of logging format that will cause syntax errors.
2. Some of the tests will not fail and need to be resolved.

[abdulbasitds]

@potiuk thank you for writing the details about the rebase, it seems very helpful. I will try it tommorow and will let you know

[abdulbasitds]

@potiuk @kaxil I re based (removed conflicts and the last command (git push --force-with-lease) according to rebase guide worked)

Can anyone guide me what just happened and what can be the next step(specially on my part) for integration?

[abdulbasitds]

Hello @kaxil and @feluelle do you have idea about the current failing test? it says something like the tests are not listed

[abdulbasitds]

Hello @kaxil and @feluelle do you have idea about the current failing test? it says something like the tests are not listed

It was due to naming of the tests.
All tests are passing now and all changes have been addressed

[paulsyl]

@abdulbasitds I would still like to see this addressed in your code.

#6007 (comment)

TIMEOUT is a very valid issue with Glue and this will keep airflow running indefinitely. Additionally, I think good practice would be to return the 'ERROR_MESSAGE' to Airflow upon a failure state. The response is only available when a Glue job has FAILED.

if job_run_state == 'FAILED':
                    self.log.info("JOB ERROR: %s", job_status['JobRun']['ErrorMessage'])
                raise AirflowException(job_message)

[abdulbasitds]

@abdulbasitds I would still like to see this addressed in your code.

#6007 (comment)

TIMEOUT is a very valid issue with Glue and this will keep airflow running indefinitely. Additionally, I think good practice would be to return the 'ERROR_MESSAGE' to Airflow upon a failure state. The response is only available when a Glue job has FAILED.

if job_run_state == 'FAILED':
                    self.log.info("JOB ERROR: %s", job_status['JobRun']['ErrorMessage'])
                raise AirflowException(job_message)

@paulsyl I catered this in the sensor, can you please see the latest commit and if looks okay than may be we can replicate that into hook?

[paulsyl]

@abdulbasitds I would still like to see this addressed in your code.
#6007 (comment)
TIMEOUT is a very valid issue with Glue and this will keep airflow running indefinitely. Additionally, I think good practice would be to return the 'ERROR_MESSAGE' to Airflow upon a failure state. The response is only available when a Glue job has FAILED.

if job_run_state == 'FAILED':
                    self.log.info("JOB ERROR: %s", job_status['JobRun']['ErrorMessage'])
                raise AirflowException(job_message)

@paulsyl I catered this in the sensor, can you please see the latest commit and if looks okay than may be we can replicate that into hook?

@abdulbasitds
The sensor looks good, I think it should be in the Hook, does the Sensor ever get executed? The function Job_Completion has a WHILE TRUE waiting for the job to finish

[abdulbasitds]

@abdulbasitds I would still like to see this addressed in your code.
#6007 (comment)
TIMEOUT is a very valid issue with Glue and this will keep airflow running indefinitely. Additionally, I think good practice would be to return the 'ERROR_MESSAGE' to Airflow upon a failure state. The response is only available when a Glue job has FAILED.

if job_run_state == 'FAILED':
                    self.log.info("JOB ERROR: %s", job_status['JobRun']['ErrorMessage'])
                raise AirflowException(job_message)

@paulsyl I catered this in the sensor, can you please see the latest commit and if looks okay than may be we can replicate that into hook?

@abdulbasitds
The sensor looks good, I think it should be in the Hook, does the Sensor ever get executed? The function Job_Completion has a WHILE TRUE waiting for the job to finish

@paulsyl I think you are confusing sensor with the hook, sensor is used to poke about the status of the Job.
Anyways, I have added timeout and also have raised exception for failed job in the hook

[kaxil]

I am going ahead and merging this one. Please feel free to raise a PR to improve this code further if that is not addressed in this PR @paulsyl and @feluelle

Looks like that can't be done as @feluelle has requested some changes so Github won't allow merging this PR until @feluelle approves :)

[kaxil]

Errors:

airflow/providers/amazon/aws/sensors/glue.py:37:5: E301 expected 1 blank line, found 0

There were some flake8 errors. Exiting

[feluelle]

The rest LGTM. I personally like to document consistenly all public methods via

The title / description

:param foo: 
:type foo: 

:returns: bar
:rtype: bar

..but I think it is fine for now :)

[feluelle]

So @kaxil go ahead and merge it after the flake8 issue has been fixed.

[feluelle]

@abdulbasitds FYI: CI Build / Quarantined:Pg9.6,Py3.6 (pull_request) fails sometimes that's why it is called like that - those tests need to be fixed sometime.

[abdulbasitds]

@abdulbasitds FYI: CI Build / Quarantined:Pg9.6,Py3.6 (pull_request) fails sometimes that's why it is called like that - those tests need to be fixed sometime.

Okay thanks, should I do git commit --allow-empty -m "Trigger Github actions, or leave it as advised by others?

[boring-cyborg]

Awesome work, congrats on your first merged pull request!

[abdulbasitds]

Thankyou @kaxil @feluelle and all for approval, merging and helping the PR to be merged.

Also, thank you @OElesin for initiating this work.

[paulsyl]

@abdulbasitds great work