Update compatibility with google-cloud-redis>=2.0.0 #13117

mik-laj · 2020-12-16T22:24:50Z

Part of: #12116

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

github-actions · 2020-12-16T22:50:47Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

turbaszek · 2020-12-16T23:02:14Z

airflow/providers/google/cloud/example_dags/example_cloud_memorystore.py

Does this suggest a breaking change?

Yes. Unfortunately yes. Upgrading the library version introduces minor behavioral differences, so we will have to add an update entry for these libraries in UPDATING.md.

CAan we keep backwards-compatibility? I think we should change our approach and only introduce backwards-incompatible changes in providers if we absolutely must and there is no other way to solve it.

This will be very problematic because this is changed in the third library and we are just passing that object as JSON. These are the consequences of the fact that our operators are only wrappers on the API and the abstraction layer between the third library and the DAG is relatively thin.

In my opinion, we need to introduce a breaking change. Otherwise, we will be forced to use old and outdated libraries that will not allow us to use new API fields/features.

I added docs.

Is the list of those modified fields known and fixed ? I guess it should be as it comes from the old version of library which is not going to be updated. I guess the values in Xcom come from another operator of ours, and they contain the values returned by CloudMemorystoreGetInstanceOperator - result of MessageToDict method call?

I guess in this case we could - potentially - make it backwards-compatible by modifying the dictionary returned and copying the values with legacy field names? How feasible it is ? Maybe in the new library there is already a fixed list of values or even a code that we could use to do it ?

The word list is not provided explicitly, but we may try to guess these names when comparing types with similar names in the new version of the library, but this will require creating a large key mapping from one version of the library to the new library. This key list is also not fixed as new versions of libraries may contain new keys.

This has its limitation. If we only replace dict keys based on a predefined list of keys, we can accidentally modify user values as well, because Google libraries not often, but sometimes also store values from the user as a key.
https://github.com/googleapis/googleapis/blob/eabe7c0fde64b1451df6ea171b2009238b0df07c/google/cloud/datacatalog/v1/tags.proto#L79
https://github.com/googleapis/googleapis/blob/eabe7c0fde64b1451df6ea171b2009238b0df07c/google/cloud/redis/v1/cloud_redis.proto#L276
For this not to happen, we may collect such cases as path, but we will still have to maintain them.

PS. Google uses the snake_case convention in protobuff, so with this change, Google tried to standardize all field names.

There is one more problem. If we try to rename the keys, then it will not be possible to create an object using this dictionary.

instance_dict = t.xcom_pull('get-instance') instance = Instance(**instance_dict)

In this case, I think we should put that change on hold and release/merge it together with other breaking changes for google provider. I do not think we have an immediate need to migrate to this new '2.0.0' version. And this should happen IMHO at the point we add more breaking changes together.

All right - after the discussion - seems like we have more breaking changes in this "batch" so this will be fine to make it backwards incompatible and set version to 2.0.0

github-actions · 2020-12-18T13:07:41Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

github-actions · 2020-12-18T13:27:27Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2020-12-20T02:10:47Z

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

potiuk · 2020-12-20T08:59:40Z

airflow/providers/google/ADDITIONAL_INFO.md

I think we need to specify version here. And I think this is a very good way to make backwards incompatible changes here- by adding version number here that indicates what kind of changes we are talking about.

potiuk · 2020-12-20T09:05:10Z

airflow/providers/google/cloud/example_dags/example_cloud_memorystore.py

Is the list of those modified fields known and fixed ? I guess it should be as it comes from the old version of library which is not going to be updated. I guess the values in Xcom come from another operator of ours, and they contain the values returned by CloudMemorystoreGetInstanceOperator - result of MessageToDict method call?

I guess in this case we could - potentially - make it backwards-compatible by modifying the dictionary returned and copying the values with legacy field names? How feasible it is ? Maybe in the new library there is already a fixed list of values or even a code that we could use to do it ?

github-actions · 2020-12-20T23:19:58Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

potiuk

This change introduces a breking change so we should release it together with other, accumulated breaking changes to avoid unnecessary number of breaking release for Google provider.

github-actions · 2020-12-21T18:34:03Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

(cherry picked from commit 0b626c8)

boring-cyborg bot added the provider:google Google (including GCP) related issues label Dec 16, 2020

turbaszek reviewed Dec 16, 2020

View reviewed changes

turbaszek approved these changes Dec 18, 2020

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 18, 2020

mik-laj requested a review from potiuk December 18, 2020 13:31

This was referenced Dec 18, 2020

Support google-cloud-datacatalog 1.0.0 #13097

Merged

Update compatibility with google-cloud-kms>=2.0 #13124

Merged

potiuk reviewed Dec 20, 2020

View reviewed changes

potiuk requested changes Dec 21, 2020

View reviewed changes

github-actions bot removed the full tests needed We need to run full set of tests for this PR to merge label Dec 21, 2020

potiuk approved these changes Dec 21, 2020

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 21, 2020

mik-laj added this to the apache-airflow-providers-google v2 milestone Dec 21, 2020

mik-laj force-pushed the redis-update branch from 40d04f1 to 92fdc7f Compare December 21, 2020 18:59

mik-laj changed the title ~~Update compatibility with google-cloud-redis==2.0.0~~ Update compatibility with google-cloud-redis>=2.0.0 Dec 21, 2020

Kamil Breguła added 6 commits December 22, 2020 13:05

A

4bec579

Update compatibility with google-cloud-redis==2.0.0

dba58e8

Add docs about breaking changes

c17539f

fixup! Add docs about breaking changes

2df26c1

fixup! fixup! Add docs about breaking changes

c6f9cd3

fixup! fixup! fixup! Add docs about breaking changes

c4b5ead

mik-laj force-pushed the redis-update branch from 92fdc7f to c4b5ead Compare December 22, 2020 12:13

mik-laj merged commit 0b626c8 into apache:master Dec 22, 2020

mik-laj deleted the redis-update branch December 22, 2020 15:25

potiuk pushed a commit that referenced this pull request Mar 3, 2021

Support google-cloud-redis>=2.0.0 (#13117)

ad61070

(cherry picked from commit 0b626c8)

potiuk pushed a commit that referenced this pull request Mar 3, 2021

Support google-cloud-redis>=2.0.0 (#13117)

ce6c631

(cherry picked from commit 0b626c8)

potiuk pushed a commit that referenced this pull request Mar 3, 2021

Support google-cloud-redis>=2.0.0 (#13117)

f2b5637

(cherry picked from commit 0b626c8)

Update compatibility with google-cloud-redis>=2.0.0 #13117

Update compatibility with google-cloud-redis>=2.0.0 #13117

Uh oh!

Conversation

mik-laj commented Dec 16, 2020

Uh oh!

github-actions bot commented Dec 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 18, 2020

Uh oh!

github-actions bot commented Dec 18, 2020

Uh oh!

github-actions bot commented Dec 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 20, 2020

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

potiuk Dec 20, 2020 •

edited

Loading

potiuk Dec 21, 2020 •

edited

Loading

potiuk Dec 20, 2020 •

edited

Loading