Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Azure Service Bus Topic (Create, Delete) Operators #25436

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

bharanidharan14
Copy link
Contributor

@bharanidharan14 bharanidharan14 commented Aug 1, 2022

  • Added AzureServiceBusTopicCreateOperator, AzureServiceBusTopicDeleteOperator to create and delete topic in azure service bus
  • Added example to create and delete topic operator in Example DAG
  • Added Unit Test case for operators

@bharanidharan14 bharanidharan14 changed the title Implement Azure Service Bus Topic create Operator Implement Azure Service Bus Topic (Create, Delete) Operators Aug 1, 2022
@bharanidharan14 bharanidharan14 force-pushed the asb-topic branch 2 times, most recently from d71893e to fa12cac Compare August 2, 2022 03:03
@bharanidharan14 bharanidharan14 marked this pull request as ready for review August 2, 2022 05:21
@bharanidharan14 bharanidharan14 force-pushed the asb-topic branch 2 times, most recently from 711920a to 2e7341c Compare August 2, 2022 09:15
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

LGTM, one small change plesae

airflow/providers/microsoft/azure/operators/asb.py Outdated Show resolved Hide resolved
airflow/providers/microsoft/azure/operators/asb.py Outdated Show resolved Hide resolved
@ashb
Copy link
Member

ashb commented Aug 3, 2022

What is the behaviour if the topic you try to create already exists? I'm assuming the underlying hook would fail with an exception, but I think the operator should catch that error so that the operator can be used idempotently: i.e. we should think of the job of this operator to ensure that the specific topic exists. (Similarly for delete operator)

@bharanidharan14
Copy link
Contributor Author

bharanidharan14 commented Aug 4, 2022

What is the behaviour if the topic you try to create already exists? I'm assuming the underlying hook would fail with an exception, but I think the operator should catch that error so that the operator can be used idempotently: i.e. we should think of the job of this operator to ensure that the specific topic exists. (Similarly for delete operator)

yes the existing flow will through an error if the topic name exists, Now I have add try catch so before creating the topic just checking if the topic exists, if it exists logging it with message and returning the topic name, similarly for delete operator

@bharanidharan14 bharanidharan14 force-pushed the asb-topic branch 2 times, most recently from 6000ff0 to 6329e3c Compare August 4, 2022 09:31
@josh-fell
Copy link
Contributor

yes the existing flow will through an error if the topic name exists, Now I have add try catch so before creating the topic just checking if the topic exists, if it exists logging it with message and returning the topic name, similarly for delete operator

I wonder if there should be a parameter for users to have the option of overwriting a topic that might exist. WDYT?

Also, for deleting a topic, if the topic doesn't exist should the task fail? Would it be misleading for a task to succeed if the operation is should perform does not occur and the user didn't explicitly choose to be indifferent about the operation? I could imagine a scenario where users expect to have topics deleted when they aren't even though "Airflow says" the deletion task succeeded. Just some thoughts.

@ashb
Copy link
Member

ashb commented Aug 4, 2022

Would it be misleading for a task to succeed if the operation is should perform does not occur and the user didn't explicitly choose to be indifferent about the operation

Not to my mind. Good tasks are idempotent, so deleting a non-existent topic has achieved the goal: to ensure that the topic does not exist.

@josh-fell
Copy link
Contributor

Not to my mind. Good tasks are idempotent, so deleting a non-existent topic has achieved the goal: to ensure that the topic does not exist.

Fair point.

@bharanidharan14
Copy link
Contributor Author

yes the existing flow will through an error if the topic name exists, Now I have add try catch so before creating the topic just checking if the topic exists, if it exists logging it with message and returning the topic name, similarly for delete operator

I wonder if there should be a parameter for users to have the option of overwriting a topic that might exist. WDYT?

Just thinking, overwriting the existing topic is kind of update operation right ?

@josh-fell
Copy link
Contributor

Just thinking, overwriting the existing topic is kind of update operation right ?

Sure, depending on how Azure Service Bus management operates and/or if someone wanted to patch a particular attribute (if that's even possible). What if you wanted to completely rebuild the topic? Yes, you could check to see if it exists, delete it, and then rebuild it, but I was thinking some analogous feature to file operations.

I'm not saying it should or even can be done. I haven't been used ASB in a long time. Just tossing spaghetti to see if it sticks. You can tell me to go back to my corner and it could be something for the future (or never) maybe.

@bharanidharan14
Copy link
Contributor Author

Just thinking, overwriting the existing topic is kind of update operation right ?

Sure, depending on how Azure Service Bus management operates and/or if someone wanted to patch a particular attribute (if that's even possible). What if you wanted to completely rebuild the topic? Yes, you could check to see if it exists, delete it, and then rebuild it, but I was thinking some analogous feature to file operations.

I'm not saying it should or even can be done. I haven't been used ASB in a long time. Just tossing spaghetti to see if it sticks. You can tell me to go back to my corner and it could be something for the future (or never) maybe.

Yes, there is an Option to patch/update the Topic, I was planning to add update topic operator as next future operator. on that the user can update whatever attribute they want(which is allowed to update).

Even I need suggestion this create topic operator should be.

  1. Only create and if it exists just return it ?
  2. Create the topic, if it exists delete it and create with new properties?

@bharanidharan14 bharanidharan14 requested review from ashb and removed request for pankajastro August 5, 2022 02:44
@bharanidharan14 bharanidharan14 force-pushed the asb-topic branch 2 times, most recently from 2e9de75 to 44ca227 Compare August 10, 2022 05:52
@bharanidharan14
Copy link
Contributor Author

Just thinking, overwriting the existing topic is kind of update operation right ?

Sure, depending on how Azure Service Bus management operates and/or if someone wanted to patch a particular attribute (if that's even possible). What if you wanted to completely rebuild the topic? Yes, you could check to see if it exists, delete it, and then rebuild it, but I was thinking some analogous feature to file operations.
I'm not saying it should or even can be done. I haven't been used ASB in a long time. Just tossing spaghetti to see if it sticks. You can tell me to go back to my corner and it could be something for the future (or never) maybe.

Yes, there is an Option to patch/update the Topic, I was planning to add update topic operator as next future operator. on that the user can update whatever attribute they want(which is allowed to update).

Even I need suggestion this create topic operator should be.

  1. Only create and if it exists just return it ?
  2. Create the topic, if it exists delete it and create with new properties?

@josh-fell

@josh-fell
Copy link
Contributor

Just thinking, overwriting the existing topic is kind of update operation right ?

Sure, depending on how Azure Service Bus management operates and/or if someone wanted to patch a particular attribute (if that's even possible). What if you wanted to completely rebuild the topic? Yes, you could check to see if it exists, delete it, and then rebuild it, but I was thinking some analogous feature to file operations.
I'm not saying it should or even can be done. I haven't been used ASB in a long time. Just tossing spaghetti to see if it sticks. You can tell me to go back to my corner and it could be something for the future (or never) maybe.

Yes, there is an Option to patch/update the Topic, I was planning to add update topic operator as next future operator. on that the user can update whatever attribute they want(which is allowed to update).
Even I need suggestion this create topic operator should be.

  1. Only create and if it exists just return it ?
  2. Create the topic, if it exists delete it and create with new properties?

@josh-fell

I think option 1 is fine for now. If users find that indeed having option 2 would be helpful, they are more than welcome to log a feature or submit a PR adding the functionality. Thinking about my suggestion, it was a little much and out of scope for the initial creation of these operators.

@bharanidharan14
Copy link
Contributor Author

@josh-fell @ashb Can you review this PR, I have addressed the review comments

Implement Azure Service Bus Topic Create, Delete Operators

Implement Azure Service Bus Topic Create, Delete Operators

Implement Azure Service Bus Topic Create, Delete Operators

Implement Azure Service Bus Topic create Operator

- Added Create Topic Operator
- Added Test case
- Added Example DAG
- Added Doc for the operator

Doc fix

Implemented Azure service bus Topic Delete operator

- Added Operator for Delete Topic Operator
- Added example DAG

Doc fix

Doc fix

Add check for the existing topic

Fix Test case

Fix doc string
@ashb ashb merged commit 5c7c518 into apache:main Aug 16, 2022
@ashb ashb deleted the asb-topic branch August 16, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants