Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-5850] Capture task logs in DockerSwarmOperator #6552

Merged
merged 3 commits into from Apr 26, 2020

Conversation

akki
Copy link
Contributor

@akki akki commented Nov 12, 2019

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-5850
    • 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 test:
    test_logging

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

@OmerJog
Copy link
Contributor

OmerJog commented Nov 12, 2019

@akki Please fix flake8 violations:


tests/operators/test_docker_swarm_operator.py:79:111: E501 line too long (119 > 110 characters)
tests/operators/test_docker_swarm_operator.py:176:111: E501 line too long (119 > 110 characters)
airflow/contrib/operators/docker_swarm_operator.py:137:111: E501 line too long (112 > 110 characters)
airflow/contrib/operators/docker_swarm_operator.py:139:43: E261 at least two spaces before inline comment
airflow/contrib/operators/docker_swarm_operator.py:146:21: F841 local variable 'e' is assigned to but never used
airflow/contrib/operators/docker_swarm_operator.py:147:111: E501 line too long (117 > 110 characters)
airflow/contrib/operators/docker_swarm_operator.py:149:21: F841 local variable 'e' is assigned to but never used
airflow/contrib/operators/docker_swarm_operator.py:150:111: E501 line too long (118 > 110 characters)

airflow/contrib/operators/docker_swarm_operator.py Outdated Show resolved Hide resolved
airflow/contrib/operators/docker_swarm_operator.py Outdated Show resolved Hide resolved
airflow/contrib/operators/docker_swarm_operator.py Outdated Show resolved Hide resolved
airflow/contrib/operators/docker_swarm_operator.py Outdated Show resolved Hide resolved
airflow/contrib/operators/docker_swarm_operator.py Outdated Show resolved Hide resolved
@akki akki force-pushed the AIRFLOW-5850 branch 3 times, most recently from 382ae5c to ee4daac Compare November 20, 2019 15:40
@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

Merging #6552 into master will increase coverage by 0.93%.
The diff coverage is 77.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6552      +/-   ##
==========================================
+ Coverage   84.32%   85.25%   +0.93%     
==========================================
  Files         676      756      +80     
  Lines       38363    39884    +1521     
==========================================
+ Hits        32348    34003    +1655     
+ Misses       6015     5881     -134
Impacted Files Coverage Δ
airflow/contrib/operators/pubsub_operator.py 100% <ø> (ø) ⬆️
...ample_dags/example_branch_python_dop_operator_3.py 75% <ø> (ø) ⬆️
...flow/contrib/example_dags/example_qubole_sensor.py 100% <ø> (ø) ⬆️
airflow/contrib/hooks/qubole_hook.py 52.67% <ø> (ø) ⬆️
...w/example_dags/example_external_task_marker_dag.py 100% <ø> (ø)
airflow/executors/local_executor.py 92.48% <ø> (ø) ⬆️
...low/contrib/operators/wasb_delete_blob_operator.py 100% <ø> (ø) ⬆️
airflow/contrib/sensors/sagemaker_tuning_sensor.py 100% <ø> (ø) ⬆️
...ontrib/example_dags/example_kubernetes_operator.py 78.57% <ø> (ø) ⬆️
...flow/contrib/sensors/sagemaker_transform_sensor.py 100% <ø> (ø) ⬆️
... and 553 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e77089...4e0e3af. Read the comment docs.

@akki
Copy link
Contributor Author

akki commented Nov 20, 2019

Getting another set of eyes always helps. Thanks @hredestig and @OmerJog for reviewing, really appreciate.

I've fixed the concerns mentioned and CI also seems to be green now. :)

@akki
Copy link
Contributor Author

akki commented Nov 20, 2019

@potiuk I am guessing you'd be the correct admin to ping for this PR - may I request your review on this?

@akki
Copy link
Contributor Author

akki commented Dec 3, 2019

Hi
Would any of the committers want to have a look at this feature/PR?

This would make debugging DockerSwarmOperator based tasks much easier.

@kaxil kaxil requested a review from dimberman December 17, 2019 03:04
@akki akki changed the title AIRFLOW-5850: Capture task logs in DockerSwarmOperator IRFLOW-5850] Capture task logs in DockerSwarmOperator Jan 23, 2020
@akki akki changed the title IRFLOW-5850] Capture task logs in DockerSwarmOperator [AIRFLOW-5850] Capture task logs in DockerSwarmOperator Jan 23, 2020
@akki akki force-pushed the AIRFLOW-5850 branch 2 times, most recently from 9d29353 to 5bc1616 Compare January 23, 2020 11:25
@akki
Copy link
Contributor Author

akki commented Jan 27, 2020

Hi Airflow team
I have rebased my work on top of master (again!) as suggested in the JIRA ticket recently. CI also seems green now. 😌

Would anyone please help review and approve this PR?

(PS - the postgres connection error in CI don't seem related to this PR)

@akki
Copy link
Contributor Author

akki commented Jan 31, 2020

Sorry, I am on vacation for a couple of weeks. Will start working on this once I come back.

@stale
Copy link

stale bot commented Apr 7, 2020

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.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 7, 2020
@mik-laj
Copy link
Member

mik-laj commented Apr 7, 2020

@akki Is this change complete? Travis is green, but I do not see discussion.

The PR author is responsible for engaging people so that this change can come in.

Ping @ #development slack, comment @people. Be annoying. Be considerate.

More info:
https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 7, 2020
@akki
Copy link
Contributor Author

akki commented Apr 8, 2020

Hi @mik-laj
Yes, this is complete.

I had made these 2 comments when I fixed everything & completed it last time:
#6552 (comment)
#6552 (comment)

Do I need to mark it as "done" somewhere else as well or do something else? Can you please tell me what else I need to do to move this PR forward and get further reviews from Airflow authors?
Would love to hear your thoughts and get this merged.

@akki
Copy link
Contributor Author

akki commented Apr 8, 2020

Sorry, now I understand the 2nd part of ur comment.

Do you mean I need to join Slack and then ping @people there to get my GitHub PR reviewed? Is it possible we use GitHub's commenting feature itself?
I think it is more convenient than signing up for Slack and pinging there each time a PR has to be reviewed.

@mik-laj
Copy link
Member

mik-laj commented Apr 8, 2020

Notifications on Github are not always effective because contributors often have notifications from many repositories configured and it's easy to miss a message in the stream.

In practice, writing on #how-to-pr or private messages seems to be much more effective. Contributors can configure the "Remind me" option so they don't forget to write back.

Copy link
Member

@mik-laj mik-laj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @dimberman Can you look again because you gave a red stamp on this PR?

@akki
Copy link
Contributor Author

akki commented Apr 8, 2020

Notifications on Github are not always effective because contributors often have notifications from many repositories configured and it's easy to miss a message in the stream.

In practice, writing on #how-to-pr or private messages seems to be much more effective. Contributors can configure the "Remind me" option so they don't forget to write back.

Cool, thanks for explaining. :) I'll keep that in mind in future.

@mik-laj
Copy link
Member

mik-laj commented Apr 8, 2020

One more information. If you have other questions, please visit the #newbie-questions Slack channel where you can ask any questions you want - it's a safe space where it is expected that people asking questions do not know a lot about Airflow (yet!). If you started worrying and didn't know what to do to get yours merged then you could ask in this channel.

@mik-laj
Copy link
Member

mik-laj commented Apr 8, 2020

Community Over Code: the maxim "Community Over Code" is frequently reinforced throughout the Apache community, as the ASF asserts that a healthy community is a higher priority than good code. Strong communities can always rectify problems with their code, whereas an unhealthy community will likely struggle to maintain a codebase in a sustainable manner. More on healthy community development.

https://www.apache.org/theapacheway/

@dimberman dimberman added this to the Airflow 1.10.11 milestone Apr 26, 2020
@dimberman dimberman merged commit 3237c7e into apache:master Apr 26, 2020
@akki akki deleted the AIRFLOW-5850 branch April 26, 2020 16:22
@kaxil kaxil removed this from the Airflow 1.10.13 milestone Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants