Skip to content

[AIRFLOW-2463] Make task instance context available for hive queries - #3405

Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom
KevinYang21:kevin_yang_add_context
Closed

[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom
KevinYang21:kevin_yang_add_context

Conversation

@KevinYang21

@KevinYang21KevinYang21 commented May 23, 2018•
edited
Loading

Copy link
Copy Markdown
Member

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

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

This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:

  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.

This change will make task instance context available in the following cases:

  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable

The following cases are already covered without this change (with updated names):

  • HiveCliHook() calls in HiveOperator(): as hive_conf

The following cases are not cover even with this change:

  • beeline calls in BashOperator.

NOTE:
If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.
If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    -- tests/operators/bash_operator.py
    -- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    -- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    -- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf

HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.

This logic has been running in Airbnb production for a week.

airflow test output:
hive_cli task:

[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:

[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:

[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:

[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch 8 times, most recently from e830fb5 to 9815017CompareMay 24, 2018 00:53
@codecov-io

codecov-io commented May 24, 2018•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #3405 into master will increase coverage by 0.01%.
The diff coverage is 93.22%.

Impacted file tree graph

@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️

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 b0061f1...35345fd. Read the comment docs.

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch 9 times, most recently from 7ff263e to fc7467fCompareMay 24, 2018 07:24
@Fokko

Fokko commented May 24, 2018

Copy link
Copy Markdown
Contributor

Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.

@gglanzani Hive master, can you take a peek?

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch from fc7467f to 266a27aCompareMay 24, 2018 08:56
@KevinYang21

KevinYang21 commented May 24, 2018

Copy link
Copy Markdown
MemberAuthor

Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch from 266a27a to b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21 changed the title [WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21

KevinYang21 commented May 25, 2018

Copy link
Copy Markdown
MemberAuthor

PTAL @saguziel@Fokko@afernandez

KevinYang21
KevinYang21 commented May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Needed to make beeline hook work.

@gglanzanigglanzaniMay 25, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this is just a string, can you write it as

jdbc_url='"{}"'.format(jdbc_url)

as to avoid escaping?

@KevinYang21KevinYang21May 25, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

sure thing, will update

Comment threadairflow/hooks/hive_hooks.py Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch from b8237aa to 3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani reviewed May 26, 2018
View reviewed changes

@gglanzanigglanzani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

WIP review

Comment threadairflow/hooks/hive_hooks.py Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch 3 times, most recently from 8476d26 to 8d0f72fCompareMay 29, 2018 22:02
@Fokko

Fokko commented Jun 9, 2018

Copy link
Copy Markdown
Contributor

The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.

@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch 2 times, most recently from b9222c8 to 1eeba1bCompareJune 10, 2018 04:13
@KevinYang21

KevinYang21 commented Jun 10, 2018

Copy link
Copy Markdown
MemberAuthor

@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.

@KevinYang21KevinYang21 changed the title [AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch 11 times, most recently from 94d1c56 to be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21 commented Jun 28, 2018
View reviewed changes

@KevinYang21KevinYang21 left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.

@Fokko@gglanzani@saguziel PTAL

Comment threadairflow/hooks/hive_hooks.py Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key

@gglanzanigglanzaniJun 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?

@KevinYang21KevinYang21Jun 29, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.

@KevinYang21KevinYang21Jul 3, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ping

Comment threadtests/hooks/test_hive_hook.py Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?

@gglanzanigglanzaniJun 28, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as

[(1,), (2,)]

I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.

The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.

The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).

Is it possible to do it here? I think this text would suffice:

HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.

@Fokko: Agree?

@KevinYang21KevinYang21Jun 29, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.

@KevinYang21KevinYang21 changed the title [WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed the kevin_yang_add_context branch from be2ad65 to 35345fdCompareJune 28, 2018 10:11
@KevinYang21

KevinYang21 commented Jul 3, 2018

Copy link
Copy Markdown
MemberAuthor

ping

1 similar comment
@KevinYang21

KevinYang21 commented Jul 11, 2018

Copy link
Copy Markdown
MemberAuthor

ping

@gglanzani

gglanzani commented Jul 11, 2018

Copy link
Copy Markdown
Contributor

@Fokko lgtm

Fokko
Fokko approved these changes Jul 11, 2018
View reviewed changes

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond mentioned this pull request Apr 30, 2019
Merged
6 tasks
@feng-taofeng-tao mentioned this pull request Oct 22, 2019
Merged
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes
+1 more reviewer
@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
[AIRFLOW-2463] Make task instance context available for hive queries by KevinYang21 · Pull Request #3405 · apache/airflow · GitHub
Skip to content
[AIRFLOW-2463] Make task instance context available for hive queries - #3405
Closed
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Closed
[AIRFLOW-2463] Make task instance context available for hive queries#3405
KevinYang21 wants to merge 2 commits into
apache:masterfrom 
KevinYang21:kevin_yang_add_context
Conversation
@KevinYang21
@KevinYang21KevinYang21
commented
May 23, 2018•

edited

Loading

Copy link


Copy Markdown

Member
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
  •  Here are some details about my PR, including screenshots of any UI changes:
This PR is mainly to enable hive (e.g. through hive audit hook) to log task instance context( dag_id, task_id, dag_run_id and task instance execution_date) for hive queries. This would give users more audit and research power. For example:
  • When data in one table was polluted, people can find out what tasks using the table need to rerun.
  • Grouping hive queries by dag_id and task_id.
  • Figure out whether the DAG run of the task instance is a manual run, scheduled run or backfill run.
This change will make task instance context available in the following cases:
  • HiveServer2Hook() calls in Airflow official operators: as hive_conf
  • HiveCliHook() and HiveServer2Hook() calls in PythonOperator: as hive_conf
  • hive cli calls in BashOperator: as environment variable
The following cases are already covered without this change (with updated names):
  • HiveCliHook() calls in HiveOperator(): as hive_conf
The following cases are not cover even with this change:
  • beeline calls in BashOperator.
NOTE:

If as environment variable, context would be available as AIRFLOW_CTX_DAG_ID, AIRFLOW_CTX_TASK_ID, AIRFLOW_CTX_EXECUTION_DATE, AIRFLOW_CTX_DAG_RUN_ID.

If as hvie_conf, as airflow.ctx.dag_id, airflow.ctx.task_id, airflow.ctx.execution_date, airflow.ctx.dag_run_id. This is to follow the environment variable and hive conf naming convention.
Tests
  •  My PR adds the following unit tests OR does not need testing for this extremely good reason:
    
-- tests/operators/bash_operator.py
    
-- tests/operators/python_operator.py:PythonOperatorTest.test_echo_env_variables
    
-- tests/hooks/test_hive_hook.py: TestHiveCliHook.test_run_cli_with_hive_conf
    
-- tests/hooks/test_hive_hook.py: TestHiveServer2Hook. test_get_results_with_hive_conf
HiveServer2Hook uses impyla, which is compatible with at most thrift==0.9.3, but we are using thrift==0.11.0 in our CI. However we cannot just downgrade thrift because our new client package used in HiveCliHook, hmsclient, cannot work with thrift<=0.9.3. Thus the unit tests for HiveSever2Hook are removed and tested manually instead.
This logic has been running in Airbnb production for a week.
airflow test output:

hive_cli task:
[2018-05-17 02:22:19,103] {hive_hooks.py:243} INFO - airflow.ctx.dag_id=kevin_hive_s2_test
[2018-05-17 02:22:19,105] {hive_hooks.py:243} INFO - airflow.ctx.dag_run_id=
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.task_id=hive_cli
[2018-05-17 02:22:19,106] {hive_hooks.py:243} INFO - airflow.ctx.execution_date=2018-05-10T00:00:00

beeline task:
[2018-05-17 02:22:50,442] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_id;
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,512] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - | airflow.ctx.dag_id=kevin_hive_s2_test |
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - +----------------------------------------+--+
[2018-05-17 02:22:50,513] {hive_hooks.py:243} INFO - 1 row selected (0.07 seconds)
[2018-05-17 02:22:50,514] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.dag_run_id;
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,516] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - | airflow.ctx.dag_run_id= |
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - +--------------------------+--+
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 1 row selected (0.002 seconds)
[2018-05-17 02:22:50,517] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.task_id;
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,520] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - | airflow.ctx.task_id=beeline |
[2018-05-17 02:22:50,521] {hive_hooks.py:243} INFO - +------------------------------+--+
[2018-05-17 02:22:50,526] {hive_hooks.py:243} INFO - 1 row selected (0.009 seconds)
[2018-05-17 02:22:50,527] {hive_hooks.py:243} INFO - 0: jdbc:hive2://i-0e2d3cb2c448a2300:10000/def> set airflow.ctx.execution_date;
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - | set |
[2018-05-17 02:22:50,530] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - | airflow.ctx.execution_date=2018-05-10T00:00:00 |
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - +-------------------------------------------------+--+
[2018-05-17 02:22:50,531] {hive_hooks.py:243} INFO - 1 row selected (0.003 seconds)

hive_in_python_op task:
[2018-05-17 02:23:26,762] {python_operator.py:92} INFO - Exporting the following env vars: AIRFLOW_CTX_TASK_ID=hive_in_python_op
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-15 08:17:28,011] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
[2018-05-15 08:17:32,560] {hive_hooks.py:246} INFO - AIRFLOW_CTX_DAG_RUN_ID=
[2018-05-15 08:17:37,111] {hive_hooks.py:246} INFO - AIRFLOW_CTX_TASK_ID=hive_in_python_opp
[2018-05-15 08:17:41,725] {hive_hooks.py:246} INFO - AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
OK
Time taken: 0.917 seconds
airflow.ctx.dag_id=kevin_hive_s2_test
OK
Time taken: 0.892 seconds
airflow.ctx.dag_run_id=
OK
Time taken: 0.869 seconds
airflow.ctx.task_id=hive_in_python_op
OK
Time taken: 0.895 seconds
airflow.ctx.execution_date=2018-05-10T00:00:00

echo_env_vars task:
[2018-05-17 02:25:53,249] {bash_operator.py:79} INFO - Exporting the following env vars:
AIRFLOW_CTX_TASK_ID=echo_env_vars
AIRFLOW_CTX_DAG_ID=kevin_hive_s2_test
AIRFLOW_CTX_EXECUTION_DATE=2018-05-10T00:00:00
[2018-05-17 02:25:53,259] {bash_operator.py:101} INFO - Output:
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - kevin_hive_s2_test
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - 2018-05-10T00:00:00
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - echo_env_vars
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO -
[2018-05-17 02:25:53,261] {bash_operator.py:105} INFO - /home/kevin_yang/airflow
[2018-05-17 02:25:53,262] {bash_operator.py:108} INFO - Command exited with return code 0

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
    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.

    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
Code Quality
  •  Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
8 times, most recently
from
e830fb5 to
9815017CompareMay 24, 2018 00:53
@codecov-io
codecov-io
commented
May 24, 2018•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #3405 into master will increase coverage by 0.01%.

The diff coverage is 93.22%.
Impacted file tree graph
@@ Coverage Diff @@## master #3405 +/- ##
==========================================
+ Coverage 77.45% 77.46% +0.01% 
==========================================
Files 204 204 Lines 15299 15326 +27 ==========================================
+ Hits 11850 11873 +23 - Misses 3449 3453 +4
Impacted FilesCoverage Δ
airflow/operators/hive_to_samba_operator.py0% <0%> (ø)⬆️
airflow/operators/hive_to_mysql.py0% <0%> (ø)⬆️
airflow/operators/python_operator.py95.03% <100%> (+0.12%)⬆️
airflow/hooks/hive_hooks.py75.26% <100%> (+0.6%)⬆️
airflow/utils/operator_helpers.py100% <100%> (ø)⬆️
airflow/operators/bash_operator.py91.37% <100%> (+0.99%)⬆️
airflow/utils/state.py93.33% <0%> (-3.34%)⬇️
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 b0061f1...35345fd. Read the comment docs.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
9 times, most recently
from
7ff263e to
fc7467fCompareMay 24, 2018 07:24
@Fokko
Fokko
commented
May 24, 2018

Copy link


Copy Markdown

Contributor
Could you rebase against master? I see a lot of changes, also some files that look unrelated, like the python operator.
@gglanzani Hive master, can you take a peek?
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
fc7467f to
266a27aCompareMay 24, 2018 08:56
@KevinYang21
KevinYang21
commented
May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Thank you @Fokko for taking a look. I was working on making the PR pass the CI, will update the PR description later to provide more context on this change.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
266a27a to
b8237aaCompareMay 24, 2018 09:23
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesMay 24, 2018
@KevinYang21
KevinYang21
commented
May 25, 2018

Copy link


Copy Markdown

MemberAuthor
PTAL @saguziel@Fokko@afernandez
KevinYang21
KevinYang21
commented
May 25, 2018
View reviewed changes
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work.
@gglanzanigglanzaniMay 25, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Since this is just a string, can you write it as
jdbc_url='"{}"'.format(jdbc_url)
as to avoid escaping?
@KevinYang21KevinYang21May 25, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

sure thing, will update
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21May 24, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Needed to make beeline hook work properly (otherwise last statement won't be executed).
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
b8237aa to
3b7ca79CompareMay 26, 2018 01:25
gglanzani
gglanzani
reviewed
May 26, 2018
View reviewed changes
@gglanzanigglanzani
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

WIP review
Comment threadairflow/hooks/hive_hooks.py
Outdated

@gglanzanigglanzaniMay 26, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Call me a moron, but I prefer dict comprehension here
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
3 times, most recently
from
8476d26 to
8d0f72fCompareMay 29, 2018 22:02
@Fokko
Fokko
commented
Jun 9, 2018

Copy link


Copy Markdown

Contributor
The most important thing is that we don't change current behaviour, we can extend it, but it should be backwards compatible.
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
2 times, most recently
from
b9222c8 to
1eeba1bCompareJune 10, 2018 04:13
@KevinYang21
KevinYang21
commented
Jun 10, 2018

Copy link


Copy Markdown

MemberAuthor
@Fokko I moved the behavior changes in bash operator out of this PR and create this PR just for that. I listed the reason why I think we should make this change there in the PR description, let's move the discussion on that there.
@KevinYang21KevinYang21
changed the title
[AIRFLOW-2463] Make task instance context available for hive queries[WIP][AIRFLOW-2463] Make task instance context available for hive queriesJun 19, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
11 times, most recently
from
94d1c56 to
be2ad65CompareJune 28, 2018 09:17
KevinYang21
KevinYang21
commented
Jun 28, 2018
View reviewed changes
@KevinYang21KevinYang21
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Rebased to pyhive, also added unit test for hive server2 hook since the blocking dependency issue was lifted by switching to pyhive.
@Fokko@gglanzani@saguziel PTAL
Comment threadairflow/hooks/hive_hooks.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani adding the condition here for queries like set key
@gglanzanigglanzaniJun 28, 2018

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 Pardon my ignorance – I couldn't get Google to help me either – what kind of queries do start with set key?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I meant those query to get variable value like 'set airflow.ctx.task_id' would return the value set by set airflow.ctx.task_id=some_id.
@KevinYang21KevinYang21Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ping
Comment threadtests/hooks/test_hive_hook.py
Outdated

@KevinYang21KevinYang21Jun 28, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

@gglanzani had to extract the actually result string from a result tuple after rebased to pyhive, while we use to get the result string directly when using impyla. Should we be concerned about this change or make it to UPDATING.md( or maybe u have done that and I missed)?
@gglanzanigglanzaniJun 28, 2018•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

@yrqls21 I understand what you mean. PyHive returns indeed always tuples, and I call list on them, i.e. we always get structures as
[(1,), (2,)]
I did not know that Impyla returned a list of elements in the above case. I don't have a working impyla installation to test that.
The DB 2 API (that Impyla should have been following and PyHive is following) prescribes that the results should be a sequence of sequences. We are doing the proper thing here IMHO.
The fact remains that this behaviour might be unexpected when updating, so I guess we need to update UPDATING.md (I haven't done it).
Is it possible to do it here? I think this text would suffice:
HiveServer2Hook.get_results() always returns a list of tuples, even when a single column is queried, as per Python API 2.
@Fokko: Agree?
@KevinYang21KevinYang21Jun 29, 2018

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I agree if that's what DB2 API specifies.
@KevinYang21KevinYang21
changed the title
[WIP][AIRFLOW-2463] Make task instance context available for hive queries[AIRFLOW-2463] Make task instance context available for hive queriesJun 28, 2018
@KevinYang21
KevinYang21force-pushed
the
kevin_yang_add_context
branch
from
be2ad65 to
35345fdCompareJune 28, 2018 10:11
@KevinYang21
KevinYang21
commented
Jul 3, 2018

Copy link


Copy Markdown

MemberAuthor
ping

1 similar comment

@KevinYang21
KevinYang21
commented
Jul 11, 2018

Copy link


Copy Markdown

MemberAuthor
ping
@gglanzani
gglanzani
commented
Jul 11, 2018

Copy link


Copy Markdown

Contributor
@Fokko lgtm
Fokko
Fokko
approved these changes
Jul 11, 2018
View reviewed changes
@FokkoFokko
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM
lxneng pushed a commit
to lxneng/incubator-airflow
that referenced
this pull request
Aug 10, 2018
@KevinYang21@lxneng
[AIRFLOW-2463] Make task instance context available for hive queries
3becacc
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
aliceabe pushed a commit
to aliceabe/incubator-airflow
that referenced
this pull request
Jan 3, 2019
@KevinYang21Alice Berard
[AIRFLOW-2463] Make task instance context available for hive queries
fd056ee
[AIRFLOW-2463] Make task instance context
available for hive queries
update UPDATING.md, please squash
Closesapache#3405 from yrqls21/kevin_yang_add_context
@r-richmondr-richmond
mentioned this pull request
Apr 30, 2019
 Merged

6 tasks
@feng-taofeng-tao
mentioned this pull request
Oct 22, 2019
 Merged

5 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes

+1 more reviewer

@gglanzanigglanzanigglanzani left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@KevinYang21@codecov-io@Fokko@gglanzani