[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next… - #5565

Closed
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899
Closed

[AIRFLOW-4899] Fix get_dataset_list from bigquery hook to return next…#5565
benjamingrenier wants to merge 1 commit into
apache:masterfrom
benjamingrenier:AIRFLOW-4899

Conversation

@benjamingrenier

Copy link
Copy Markdown
Contributor

Next page token used and max results parameter too. The method get_datasets_list can return more than 50 datasets.

@eladkal

Copy link
Copy Markdown
Contributor

Maybe out of scope but it still won't necessarily give all datasets.
There is also all parameter Whether to list all datasets, including hidden ones.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@benjamingrenier

benjamingrenier commented Jul 11, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for your comment. I added the parameter all, it can be helpful.

Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-lajmik-laj added the provider:google Google (including GCP) related issues label Jul 22, 2019
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

@mik-laj

Copy link
Copy Markdown
Member

Are you planning to continue working on this change? This week, I would like to deal with PR reviews related to GCP. I would be happy if you would respond to all comments

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes I will work on this change soon, but I have some mistakes with my docker to run unit tests locally.
I use the same plan as describe in Contributing.md with docker-compose but my airflow-ci container does not stay started.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Travis is sad

======================================================================
13) FAIL: test_get_datasets_list (tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations)
----------------------------------------------------------------------
Traceback (most recent call last):
tests/contrib/hooks/test_bigquery_hook.py line 704 in test_get_datasets_list
self.assertEqual(result, expected_result['datasets'])
AssertionError: Lists differ: [[{'kind': 'bigquery#dataset', 'location':[289 chars]'}}]] != [{'kind': 'bigquery#dataset', 'location': [287 chars]t'}}]
First differing element 0:
[{'kind': 'bigquery#dataset', 'location':[288 chars]t'}}]
{'kind': 'bigquery#dataset', 'location': [119 chars]st'}}
Second list contains 1 additional elements.
First extra element 1:
{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}
Diff is 910 characters long. Set self.maxDiff to None to see it.
-------------------- >> begin captured logging << --------------------
airflow.contrib.hooks.bigquery_hook.BigQueryBaseCursor: INFO: Datasets List: [[{'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_2_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_2_test'}}, {'kind': 'bigquery#dataset', 'location': 'US', 'id': 'your-project:dataset_1_test', 'datasetReference': {'projectId': 'your-project', 'datasetId': 'dataset_1_test'}}]]
--------------------- >> end captured logging << ---------------------

I ran unit tests locally and all succeed for tests.contrib.hooks.test_bigquery_hook.TestDatasetsOperations.
I do not understand why Travis fails. Have you got an idea ?

Sorry, I found my mistake. I append an array in array. I fixed it.

@mik-lajmik-lajAug 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about using the list_next method here? This will simplify the code.
Example:
https://github.com/apache/airflow/blob/master/airflow/contrib/hooks/gcp_transfer_hook.py#L201-L206

@mik-laj

Copy link
Copy Markdown
Member

@benjamingrenier flake8 is sad ;_;

@benjamingrenier
benjamingrenierforce-pushed the AIRFLOW-4899 branch 3 times, most recently from ea7560e to 06dae40CompareAugust 20, 2019 12:30
Comment threadairflow/contrib/hooks/bigquery_hook.py Outdated
@mik-laj

Copy link
Copy Markdown
Member

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi. I have prepared a commit for you.
This deletes excessive logs, but it also add missing tests.
Can you take a look at this? To add this commit to the your repository, run the following command

curl https://termbin.com/fzyt | git am

Thanks it's done.

@mik-laj

Copy link
Copy Markdown
Member

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.

I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?

Cheers

Reference:
#5907

Ok I will do that.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Hi.
I made a change in the base class - GoogleCloudBaseHook. Your PR may need to be changed. Could you do rebase?
Cheers
Reference:
#5907

Ok I will do that.

What rebase do you excepted ? On airflow/master ?

@mik-laj

Copy link
Copy Markdown
Member

Yes. Please do rebase on latest apache/master.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Yes. Please do rebase on latest apache/master.

Thanks, but how do you proceed to make that properly ?

@potiuk

Copy link
Copy Markdown
Member

See CONTRIBUTING guide: https://github.com/apache/airflow/blob/master/CONTRIBUTING.md#pull-request-guidelines - it has the link explaining how to rebase.

@potiuk

Copy link
Copy Markdown
Member

Sam here @benjamingrenier -> we want to play nice and move the operators only after we merge outstanding requests so it would be great if you can rebase this PR on top of apache/master.

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will approve after rebase.

@benjamingrenier

benjamingrenier commented Sep 7, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@potiuk

Copy link
Copy Markdown
Member

Same here @benjamingrenier :(. Rebase is needed.

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Will approve after rebase.

Rebase done, but I don't understand why there are errors ?

@stale

staleBot commented Nov 20, 2019

Copy link
Copy Markdown

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

@stalestaleBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019
@stalestaleBot closed this Nov 27, 2019
@OmerJog

Copy link
Copy Markdown
Contributor

@potiuk wasn't this mergable? the failed test was just a timeout.
What was missing here?

@OmerJog

Copy link
Copy Markdown
Contributor

@benjamingrenier did you stop working on this PR ? seems like it was marked stale by mistake

@benjamingrenier

benjamingrenier commented Dec 10, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@benjamingrenier

Copy link
Copy Markdown
ContributorAuthor

Done : #6780

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

Labels

provider:googleGoogle (including GCP) related issuesstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@benjamingrenier@eladkal@mik-laj@potiuk@OmerJog