Skip to content

Refactor the Design of AzureSynapsePipeline class - #36622

Closed
ambika-garg wants to merge 6 commits into
apache:mainfrom
ambika-garg:feature-AzureSynapseRunPipelineOperator-2
Closed

Refactor the Design of AzureSynapsePipeline class#36622
ambika-garg wants to merge 6 commits into
apache:mainfrom
ambika-garg:feature-AzureSynapseRunPipelineOperator-2

Conversation

@ambika-garg

Copy link
Copy Markdown
Contributor

This PR intends to:

  1. Introduce BaseAzureSynapseClass as the parent class for better code maintainability in Azure Synapse Hooks.
    The base class will manage the common connection form, enabling child classes to inherit and share this connection form while implementing individual functionalities.

  2. Add managed identity support to DefaultAzureCredential.

@ambika-garg

Copy link
Copy Markdown
Contributor Author

While working on this PR, I encountered a warning indicating that two classes, namely BaseAzureSynapseHook and AzureSynapseHook in the synapse.py file, share a common connection type that's already registered, specifically conn_type = azure_synapse.

To resolve this issue, I propose deprecating the AzureSynapseHook class and having it inherit from BaseAzureSynapseHook. This way, it can inherit the common conn_type from its parent.

My question is whether there are any specific protocols or best practices to follow when deprecating a class.
Should I prioritize resolving this warning, or is it acceptable to proceed without addressing it in the pull request I'm working on?

Comment thread airflow/providers/microsoft/azure/hooks/synapse.py Outdated
Comment thread airflow/providers/microsoft/azure/hooks/synapse.py Outdated
Comment thread airflow/providers/microsoft/azure/hooks/synapse.py Outdated
@ambika-garg
ambika-garg force-pushed the feature-AzureSynapseRunPipelineOperator-2 branch from f8bfd22 to 7b8ca99 Compare January 9, 2024 22:54
* Introduce a new base class to support common connection to Azure Synapse
* Improves code maintainability
* Update Provider.yaml to support BaseAzureSynapseHookClass
  * Reset the synapse pipeline connection type
@ambika-garg
ambika-garg force-pushed the feature-AzureSynapseRunPipelineOperator-2 branch from 7b8ca99 to de369d0 Compare January 13, 2024 03:00
:param azure_synapse_workspace_dev_endpoint: The Azure Synapse Workspace development endpoint.
"""

conn_type: str = "azure_synapse_pipeline"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

could this lead to a breaking change? Is deprecation necessary before implementing the change?

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.

yes, it is always good to deprecate it before removing it. Breaking change gets released in only major release

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can decide on Major release for Azure provider if there is good argument in favor of it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm considering reverting this change and instead adding a deprecation warning for the upcoming release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you guide me through the process of introducing this breaking change and ensuring a smooth transition for users in the major release?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To introduce breaking change you need to add 9.0.0 in provider.yaml:

And at the top of the change log:

https://github.com/apache/airflow/blob/main/airflow/providers/microsoft/azure/CHANGELOG.rst#changelog

Add 9.0.0 with braking change title
in the content you need to explain the change and provide instructions for users how to migrate from the previous version to the current version. You can see the changelog file for examples for previous major versions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ambika-garg do you need help on this?

@github-actions

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 13, 2024
@github-actions github-actions Bot closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:microsoft-azure Azure-related issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants