Skip to content

Conversation

@lindsable
Copy link
Contributor

@lindsable lindsable commented Sep 10, 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.

Description

  • The previous version of the hook returned up to 1000 results. The update allows the user to set the max results (default and max constrained by the api is 1000) and also the NextTokenId.
  • The client request token in the run_query function is now truly optional. Boto3 doesn't allow you to send in parameters that are set to None.
  • Another function was added to return a paginator for the query results.

Tests

  • My PR adds the following unit tests:
    tests/providers/amazon/aws/hooks/test_athena.py

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

Code Quality

  • Passes flake8

@mik-laj mik-laj added the provider:amazon AWS/Amazon - related issues label Sep 11, 2019
@codecov-io
Copy link

codecov-io commented Dec 4, 2019

Codecov Report

Merging #6075 into master will decrease coverage by 0.11%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6075      +/-   ##
==========================================
- Coverage   86.52%   86.41%   -0.12%     
==========================================
  Files         874      878       +4     
  Lines       40920    41181     +261     
==========================================
+ Hits        35406    35585     +179     
- Misses       5514     5596      +82
Impacted Files Coverage Δ
airflow/providers/amazon/aws/hooks/athena.py 90.24% <90.47%> (+23.05%) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0%> (-45.08%) ⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 69.38% <0%> (-25.52%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
airflow/utils/sqlalchemy.py 84.93% <0%> (-11.74%) ⬇️
airflow/providers/ssh/hooks/ssh.py 87.71% <0%> (-0.78%) ⬇️
airflow/jobs/base_job.py 91.6% <0%> (-0.6%) ⬇️
airflow/cli/commands/task_command.py 72.12% <0%> (-0.17%) ⬇️
... and 17 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 ed2f3dc...075b008. Read the comment docs.

@lindsable lindsable requested review from feluelle and mik-laj December 5, 2019 15:14
@lindsable lindsable requested review from ashb and feluelle February 13, 2020 21:58
@lindsable lindsable requested a review from feluelle February 14, 2020 21:57
@lindsable
Copy link
Contributor Author

Let me know if you want me to squash any of my commits or rebase with master

@feluelle
Copy link
Member

I think it is fine.

If @ashb is also fine with it, he can Squash and merge it. He requested changes so I would like him to do the last review. :)

@ashb
Copy link
Member

ashb commented Feb 27, 2020

I'll take a look at this tomorrow

@dephusluke
Copy link

I took the core from it and created my own custom hook and it seems to work.
Once this will be release I'll switch to the official hook

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Sorry it took rather more than the promised "tomorrow".

@mik-laj
Copy link
Member

mik-laj commented May 3, 2020

@lindsable Do you need any help?

If you have applied all the comments of the reviewer, then it is worth asking to review again using a comment or PM on Slack. The commiters have a lot of work and are unable to check the status of every change.

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

https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#id33

@mik-laj mik-laj removed their request for review May 3, 2020 17:34
@lindsable
Copy link
Contributor Author

Sorry for the delay but I was able to rebase today and get all of the tests passing! @ashb Looks like everything is ready to merge!

@lindsable lindsable requested a review from ashb May 13, 2020 18:50
@stale
Copy link

stale bot commented Jun 28, 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 Jun 28, 2020
@stale stale bot closed this Jul 5, 2020
@lindsable
Copy link
Contributor Author

@ashb I'm sure things have been busy for you organizing the summit, but I'd really like to get this merged. Please let me know what I can do.

@ashb ashb reopened this Jul 7, 2020
@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 7, 2020
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

A few tiny changes, this is looking good.

Sorry it's been open for so very long!

make client request token optional

add optional params NextToken and MaxResults to get_query_results

add get_query_results_paginator

Stop query execution if it times out

add tests for AWSAthenaHook

remove 'tests/providers/amazon/aws/hooks/test_athena.py' from missing tests

add docs for new parameters

update tests

Don't stop query if polling reaches max tries

small improvements
@lindsable lindsable requested a review from ashb July 7, 2020 21:08
@ashb ashb merged commit 07b8102 into apache:master Jul 8, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 8, 2020

Awesome work, congrats on your first merged pull request!

manesioz added a commit to manesioz/airflow that referenced this pull request Jul 16, 2020
* Tests should also be triggered when there is just setup.py change (apache#9690)

So far tests were not triggered when only requirements changed,
but this is quite needed in fact.

* Update FlaskAppBuilder to v3 (apache#9648)

* Some Pylint fixes in airflow/models/taskinstance.py (apache#9674)

* Update migrations to ensure compatibility with Airflow 1.10.* (apache#9660)

closes apache#9640

* Fix _process_executor_events method to use in-memory try_number (apache#9692)

* use the correct claim name in the webserver (apache#9688)

* Update Thumbtack points of contact in Airflow Users list (apache#9701)

The previously-listed person is no longer at the company

* generate go client from openapi spec (apache#9502)

* generate go client from openapi spec

* move openapi codegen to seperate workflow

* [AIRFLOW-XXXX] Remove unnecessary docstring in AWSAthenaOperator

* Add health API endpoint  (apache#8144) (apache#9277)

* Add AWS StepFunctions integrations to the aws provider (apache#8749)

* Move gcs & wasb task handlers to their respective provider packages (apache#9714)

* Allow AWSAthenaHook to get more than 1000/first page of results (apache#6075)

Co-authored-by: Dylan Joss <[email protected]>

* Add Dag Runs CRUD endpoints (apache#9473)

* Make airflow/migrations/env.py Pylint Compatible (apache#9670)

* Get Airflow configs with sensitive data from Secret Backends (apache#9645)

* YAML file supports extra json parameters (apache#9549)

Co-authored-by: Kamil Breguła <[email protected]>
Co-authored-by: Vinay <[email protected]>
Co-authored-by: Kamil Breguła <[email protected]>

* fix grammar in prereq tasks gcp operator docs (apache#9728)

* Add The Climate Corporation to user list (apache#9726)

* Add Qingping Hou to committers list (apache#9725)

* Add new fantastic team member of Polidea. (apache#9724)

* Error in description after deployment (apache#9723)

* Error in description after deployment
Co-authored-by: Daniel Debny <[email protected]>

* Skip one version of Python for each test.

Skip one version of Python for each test.

* Add read-only endpoints for DAG Model (apache#9045)

Co-authored-by: Tomek Urbaszek <[email protected]>
Co-authored-by: Tomek Urbaszek <[email protected]>

* Ensure Kerberos token is valid in SparkSubmitOperator before running `yarn kill` (apache#9044)

do a kinit before yarn kill if keytab and principal is provided

* Update example DAG for AI Platform operators (apache#9727)

* Fix warning about incompatible plugins (apache#9704)

One condition was bad and warns when the plugin is for admin and FAB flask.

* Update local_task_job.py (apache#9746)

Removing the suicide joke.

* Tests are working for newly added backport providers (apache#9739)

* Tests are working for newly added backport providers

* Pre-create Celery db result tables before running Celery worker (apache#9719)

Otherwise at large scale this can end up with some tasks failing as they
try to create the result table at the same time.

This was always possible before, just exceedingly rare, but in large
scale performance testing where I create a lot of tasks quickly
(especially in my HA testing) I hit this a few times.

This is also only a problem for fresh installs/clean DBs, as once these
tables exist the possible race goes away.

This is the same fix from apache#8909, just for runtime, not test time.

* Support extra config options for Sentry (apache#8911)

For now only dsn can be configured through the airflow.cfg. Need support 'http_proxy' option for example (it can't be configured through the environment variables). This change implements solution for supporting all existed (and maybe future) options for sentry configuration.

* Use namedtuple for TaskInstanceKeyType (apache#9712)

* Use namedtuple for TaskInstanceKeyType

* Add TargetQueryValue to KEDA Autoscaler (apache#9748)

Co-authored-by: Daniel Imberman <[email protected]>

* Add unit tests for mlengine_operator_utils (apache#9702)

* Mask other forms of password arguments in SparkSubmitOperator (apache#9615)

This is a follow-up to apache#6917 before modifying the masking code.
Related: apache#9595.

* Use absolute paths in howto guides (apache#9758)

* Fix StackdriverTaskHandler + add system tests (apache#9761)

Co-authored-by: Tomek Urbaszek <[email protected]>
Co-authored-by: Tomek Urbaszek <[email protected]>

* Check project structure in sensors/transfers directories (apache#9764)

* Add tests for yandex hook (apache#9665)

* improve type hinting for celery provider (apache#9762)

* Add ME-Br to who uses Airflow list (apache#9770)

* Add 1.10.11 Changelog & Update UPDATING.md (apache#9757)

* Links Breeze documentation to new Breeze video (apache#9768)

* Fix is_terminal_support_colors functtion (apache#9734)

* Add type hinting for discord provider (apache#9773)

* Fix typo in the word "Airflow" (apache#9772)

* Add Google Stackdriver link (apache#9765)

* Improve type hinting to provider microsoft  (apache#9774)

* Unit tests jenkins hook (apache#9767)

* Fixes failing formatting of DAG file containing {} in docstring (apache#9779)

* Upgrade to latest isort (5.0.8) (apache#9782)

* Add API Endpoint - DagRuns Batch (apache#9556)

Co-authored-by: Ephraim Anierobi <[email protected]>

* Improve typing coverage in scheduler_job.py (apache#9783)

* Enable pretty output in mypy (apache#9785)

* provide_session keep return type (apache#9787)

* Refactor Google operators guides (apache#9766)

* Refactor Google guides

* fixup! Refactor Google guides

* fixup! fixup! Refactor Google guides

* Fix small errors in image building documentation (apache#9792)

* Backfill reset_dagruns set DagRun to NONE state (apache#9756)

* Add DAG Source endpoint (apache#9322)

* The group of embedded DAGs should be root to be OpenShift compatible (apache#9794)

* Add docs for replace_microseconds parameters in trigger DAG endpoint (apache#9793)

* Add multiple file upload functionality to GCS hook (apache#8849)

Co-authored-by: Timothy Healy <[email protected]>

* Keep functions signatures in decorators (apache#9786)

* Use paths relative to root docs dir  in *include directives (apache#9797)

* Add Migration guide from the experimental API to the REST API (apache#9771)


Co-authored-by: Kaxil Naik <[email protected]>
Co-authored-by: Kamil Breguła <[email protected]>

* Update paths in .github/boring-cyborg.yml (apache#9799)

* Update paths in .github/boring-cyborg.yml

* fixup! Update paths in .github/boring-cyborg.yml

* Minor typo fix in OpenAPI specification (apache#9809)

* Enable annotations to be added to the webserver service (apache#9776)

* Make airflow package type check compatible (apache#9791)

* Update README to add Py 3.8 in supported versions (apache#9804)

* Remove unnecessary comprehension (apache#9805)

* Add type annotations for redis provider (apache#9815)

* Remove package.json and yarn.lock from the prod image (apache#9814)

Closes apache#9810

* For now cloud tools are not needed in CI (apache#9818)

Currently there is "unbound" variable error printed in CI logs
because of that.

* Python 3.8.4 release breaks our builds (apache#9820)

* Allow `replace` flag in gcs_to_gcs operator. (apache#9667)

* Allow `replace` flag in gcs_to_gcs operator.
If we are not replacing, list all files in the Destination GCS bucket and only keep those files which are present in Source GCS bucket and not in Destination GCS bucket

* Add kylin operator (apache#9149)

Co-authored-by: yongheng.liu <[email protected]>

* Fix SqlAlchemy-Flask failure with python 3.8.4 (apache#9821)

* Add API Reference docs (redoc) to sphinx (apache#9806)

* Add Google Deployment Manager Hook (apache#9159)

Co-authored-by: Ephraim Anierobi <[email protected]>

* Remove HTTP guide index in docs (apache#9796)

* Improve type hinting to provider cloudant (apache#9825)

Co-authored-by: Refael Y <[email protected]>

* Add option to delete by prefix to S3DeleteObjectsOperator (apache#9350)

Co-authored-by: Felix Uellendall <[email protected]>

* Add CloudVisionDeleteReferenceImageOperator  (apache#9698)

* Add note in Updating.md about the change in `run_as_user` default (apache#9822)

Until Airflow 1.10.10 the default run_as_user config (https://airflow.readthedocs.io/en/1.10.10/configurations-ref.html#run-as-user) which defaulted it to root user `0` (https://github.com/apache/airflow/blob/96697180d79bfc90f6964a8e99f9dd441789177c/airflow/contrib/executors/kubernetes_executor.py#L295-L301)

In Airflow 1.10.11 we changed it to `50000`

* Improve typing in airflow/models/pool.py (apache#9835)

* Remove global variable with API auth backend (apache#9833)

* Fix Writing Serialized Dags to DB (apache#9836)

* Update gcp to google in docs (apache#9839)

Co-authored-by: Ashwin Shankar <[email protected]>

* BigQueryTableExistenceSensor needs to specify keyword arguments (apache#9832)

* Add guide for AI Platform (previously Machine Learning Engine) Operators  (apache#9798)

* Change DAG.clear to take dag_run_state (apache#9824)

* Change DAG.clear to take dag_run_state

* fix lint

* fix tests

* assign var

* extend original clause

* Rename DagBag.store_serialized_dags to Dagbag.read_dags_from_db (apache#9838)

* Update more occurrences of gcp to google (apache#9842)

* Add Dynata to the Airflow users list (apache#9846)

* Fix S3FileTransformOperator to support S3 Select transformation only (apache#8936)

Documentation for S3FileTransformOperator states that users
can skip transformation script if S3 Select experession is
specified, but in this case the created file is always
zero bytes long.

This fix changes the behaviour, so in case of no transformation
given, the source file (a result of S3Select) is uploaded.

* Fix DagRun.conf when using trigger_dag API (apache#9853)

fixes apache#9852

* Helm chart can now place arbitrary config settings in to airflow.cfg (apache#9816)

Rather than only allowing specific pre-determined config settings, this
change allows the user to place _any_ config setting they like in the
generated airflow.cfg, including overwriting the "generated defaults".

This providers a nicer interface for the users of the chart (even if the
could already set these via the env vars).

* Fix typo in datafusion operator (apache#9859)

Co-authored-by: michalslowikowski00 <[email protected]>

* Fix Experimental API Client (apache#9849)

* Add imagePullSecrets to the create user job (apache#9802)

So that it can pull the specified image from a private registry.

* Group CI scripts in subdirectories (apache#9653)

Reviewed the scripts and removed some of the old unused ones.

Co-authored-by: Jarek Potiuk <[email protected]>
Co-authored-by: Ephraim Anierobi <[email protected]>
Co-authored-by: Kaxil Naik <[email protected]>
Co-authored-by: Tomek Urbaszek <[email protected]>
Co-authored-by: Aneesh Joseph <[email protected]>
Co-authored-by: Dylan Joss <[email protected]>
Co-authored-by: QP Hou <[email protected]>
Co-authored-by: Cooper Gillan <[email protected]>
Co-authored-by: Omair Khan <[email protected]>
Co-authored-by: chamcca <[email protected]>
Co-authored-by: lindsable <[email protected]>
Co-authored-by: Vinay G B <[email protected]>
Co-authored-by: Kamil Breguła <[email protected]>
Co-authored-by: Vinay <[email protected]>
Co-authored-by: Vismita Uppalli <[email protected]>
Co-authored-by: Jeff Melching <[email protected]>
Co-authored-by: Daniel Debny <[email protected]>
Co-authored-by: James Timmins <[email protected]>
Co-authored-by: Tomek Urbaszek <[email protected]>
Co-authored-by: Morgan Racine <[email protected]>
Co-authored-by: Ash Berlin-Taylor <[email protected]>
Co-authored-by: Bolgov Andrey <[email protected]>
Co-authored-by: Daniel Imberman <[email protected]>
Co-authored-by: Daniel Imberman <[email protected]>
Co-authored-by: chipmyersjr <[email protected]>
Co-authored-by: Jacek Kołodziej <[email protected]>
Co-authored-by: Kanthi <[email protected]>
Co-authored-by: morrme <[email protected]>
Co-authored-by: Nitai Bezerra da Silva <[email protected]>
Co-authored-by: Rafferty Chen <[email protected]>
Co-authored-by: Mauricio De Diana <[email protected]>
Co-authored-by: Guilherme Da Silva Gonçalves <[email protected]>
Co-authored-by: takunnithan <[email protected]>
Co-authored-by: Chao-Han Tsai <[email protected]>
Co-authored-by: Tobiasz Kędzierski <[email protected]>
Co-authored-by: Tim Healy <[email protected]>
Co-authored-by: Timothy Healy <[email protected]>
Co-authored-by: Adam Dobrawy <[email protected]>
Co-authored-by: Vicken Simonian <[email protected]>
Co-authored-by: Alexander Sutcliffe <[email protected]>
Co-authored-by: royberkoweee <[email protected]>
Co-authored-by: yongheng.liu <[email protected]>
Co-authored-by: yongheng.liu <[email protected]>
Co-authored-by: Sam Wheating <[email protected]>
Co-authored-by: Ephraim Anierobi <[email protected]>
Co-authored-by: rafyzg <[email protected]>
Co-authored-by: Refael Y <[email protected]>
Co-authored-by: Shoichi Kagawa <[email protected]>
Co-authored-by: Felix Uellendall <[email protected]>
Co-authored-by: ashwinshankar77 <[email protected]>
Co-authored-by: Ashwin Shankar <[email protected]>
Co-authored-by: Nathan Hadfield <[email protected]>
Co-authored-by: Neil Bhandari <[email protected]>
Co-authored-by: Mariusz Strzelecki <[email protected]>
Co-authored-by: Michał Słowikowski <[email protected]>
Co-authored-by: michalslowikowski00 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants