Skip to content

Conversation

@shervinrad100
Copy link

@shervinrad100 shervinrad100 commented Oct 30, 2024

Problem
When executing the GoogleAdsToGcsOperator I received an error that page_size is not accepted parameters by this API. According to these docs, it appears that none of the google API versions support this parameters.

Closes: #43486

Solution
removed all refs to page_size as can no longer be passed onto the API.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Oct 30, 2024
@boring-cyborg
Copy link

boring-cyborg bot commented Oct 30, 2024

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@shervinrad100 shervinrad100 requested a review from eladkal October 30, 2024 17:34
@molcay
Copy link
Contributor

molcay commented Oct 31, 2024

Hi @shervinrad100, Thank you for the contribution.

For the Google provider (and also Airflow in general), we are following a depreciation policy which states that before removal (which requires a major version bump), we need to deprecate the public parameters/hooks/operators etc. While adding the depreciation, we also need to mention the removal date (if possible alternative things to use).

Normally, we have a hook to use it for depreciation but parameter depreciation is a bit tricky. Here is an example for depreciation for the parameter.

warnings.warn("Something is deprecated and will be removed after MONTH DD, YYYY. Please use something else instead." AirflowProviderDeprecationWarning, stacklevel=2)

Please check this warning message on the unit tests side as well.

We can remove it from the underlying API call and private methods like _search to avoid API error. However the public parts (in your PR, they are the parameter of the hooks) needs to deprecate first and then remove.

Also, I wonder about operators; as far as I can see, the operator also has this page_size parameter. Are there any reasons for not changing the operators?


I saw this is your first PR on this repo but removing something is just hard. I very much appreciated your effort, thank you!

@eladkal
Copy link
Contributor

eladkal commented Nov 11, 2024

@molcay according to the description I am not sure if deprecation is needed?
It suggests that the API itself doesn't accept this parameter so what is the reasoning with deprecation first?

@molcay
Copy link
Contributor

molcay commented Nov 18, 2024

Hi @eladkal,
The documentation page says after v16 sunset (February 5, 2025), this parameter will be removed. Currently we are trying to use the latest version in our provider but users of the operator can always change the version and use an old version (like v16 or v16.1).

Considering we have approx. 3 months before sunset of v16, I think we need to give some kind of depreciation warning to the user first and then remove it. Also, removing something requires the bump of the major version, we are just trying to avoid removing something directly.

@eladkal
Copy link
Contributor

eladkal commented Nov 18, 2024

The documentation page says after v16 sunset (February 5, 2025), this parameter will be removed.

Then the statement of:

When executing the GoogleAdsToGcsOperator I received an error that page_size is not accepted parameters by this API.

by the PR author, is not accurate?

@molcay
Copy link
Contributor

molcay commented Nov 20, 2024

@eladkal, the comment is accurate for v17, v17.1 and v18 of the Google Ads API.
On the other hand, v16 and v16.1 (which are supported until February 5, 2025) support this variable.
By default we have the latest version of the API but the users can always specify the API version to use a specific API version.
After February, since the support for v16 and v16.1 will be ended, then we can remove the parameter.
IMHO, we need to put the deprecation warning for February right now and will remove it on February

@potiuk
Copy link
Member

potiuk commented Nov 28, 2024

Shall we merge it @eladkal ?

@eladkal
Copy link
Contributor

eladkal commented Nov 28, 2024

Shall we merge it @eladkal ?

According to @molcay we can't merge it unless we bump min version of SDK to 17. If we don't wish to do it then we need to deprecate the parameter and have some mechanism to detect which version of the SDK is being used to populate the right function signature.

I'll leave to Google folks to decide how they want to proceed.

@potiuk
Copy link
Member

potiuk commented Nov 28, 2024

@molcay @VladaZakharova ?

@molcay
Copy link
Contributor

molcay commented Dec 3, 2024

Hi @potiuk,

Since we have ~3 months before sunset for v16.x, we have to deprecate first and then after February we can remove it.
Some users might be using the v16.x of the API and the page_size parameter is available for them.

@shervinrad100 did you have an update on this topic?

@eladkal
Copy link
Contributor

eladkal commented Dec 3, 2024

Just to clarify, the current state is that the feature is usable only for v16 as v17+ get exception thus can't use it.
If that is the case I suggest to get this resolved before next cut of providers wave.

@molcay
Copy link
Contributor

molcay commented Dec 4, 2024

Hi @eladkal,
Yes, it is currently working in v16 but not in v17+.
Do we know when it will be the next cut?
I asked @shervinrad100 to clarify the status. According to the response we might take action.

@molcay
Copy link
Contributor

molcay commented Jan 2, 2025

Hi @eladkal,
I created a follow-up PR for this change. Can you have a look at it?

@molcay
Copy link
Contributor

molcay commented Jan 8, 2025

Hi,
The follow-up PR: #45239 is already merged! We can close this

@potiuk potiuk closed this Jan 8, 2025
@potiuk
Copy link
Member

potiuk commented Jan 8, 2025

Closing

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

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GoogleAdsToGcsOperator sends default headers which is no longer supported by the API

4 participants