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

New Tableau operator: TableauRefreshDatasourceOperator #16937

Closed

Conversation

ciancolo
Copy link
Contributor

Hello all,
I propose you a new Airflow Operator for Tableau: TableauRefreshDatasourceOperator. This new operator uses the Tableau API to refresh an existing Datasource published in a Tableau Server.

The operator is very similar to TableauRefreshWorkbookOperator, but I thought it was more clear and clean to create a new one instead to modify the existing one.

The operator is very useful to refresh data sources, not in time set way but in an asynchronous way, in my case, it is very useful to add the refresh after my ETL pipelines. I'm currently using this operator in my Airflow installation.

Changes:

  1. Created TableauRefreshDatasourceOperator
  2. Created unit test for TableauRefreshDatasourceOperator
  3. Added the operator to the provider.yaml file

Thank you.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Looks cool. Let's see the test pass but it looks good to me :)

@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jul 12, 2021

job_id = self._refresh_datasource(tableau_hook, datasource.id)
if self.blocking:
from airflow.providers.tableau.sensors.tableau_job_status import TableauJobStatusSensor
Copy link
Member

Choose a reason for hiding this comment

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

One small comment here though. It looks a bit strange to use sensor in operator. While I understand where it comes from, I would like to propose a small refactor. Why don't you extract the "poke()" internals as a separate Hook method wait_until_succeeded and use that method in both Sensor and refresh_datasource Operator?

Copy link
Contributor

@eladkal eladkal Jul 12, 2021

Choose a reason for hiding this comment

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

Sorry commented with my work account rather than my personal one :)
so removed the previous comments

One small comment here though. It looks a bit strange to use sensor in operator

This is how it's done now in TableauRefreshWorkbookOperator

Also #16916 will replace the usage of the TableauJobStatusSensor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't notice that! The most generic implementation of #16915 is cooler than mine. I will go with that!

Anyway, I had already implemented the requested changes, in a similar way of #16916 , but modified both the sensor and the hook. I pushed the implementation in case you want to use it to replace the TableauJobStatusSensor in the operators.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I looked closer and I proposed a small refactor to move waiting logic to hook.

Copy link

@elad-k elad-k left a comment

Choose a reason for hiding this comment

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

I think I think though that #16915 going to solve this in a more generic manner as it will handle all options of resource methods. WDYT?

@ciancolo ciancolo force-pushed the feature-tableau-datasource-refresh branch from b08b6a3 to 0b8cc9a Compare July 12, 2021 12:43
@eladkal
Copy link
Contributor

eladkal commented Jul 13, 2021

This PR address several points:

  1. Add functionality to TableauHook to support waiting
  2. Refactor to TableauRefreshWorkbookOperator (removing the Sensor usage)
  3. Add new TableauRefreshDatasourceOperator

Item 1-2 are independent from 3. Item 3 may not be needed if #16915 is merged.
If that's OK with you and with @potiuk I suggest to split 1-2 to another PR that we can review quickly & separately and afterwards think about 3. WDYT?

@ciancolo
Copy link
Contributor Author

This PR address several points:

  1. Add functionality to TableauHook to support waiting
  2. Refactor to TableauRefreshWorkbookOperator (removing the Sensor usage)
  3. Add new TableauRefreshDatasourceOperator

Item 1-2 are independent from 3. Item 3 may not be needed if #16915 is merged.
If that's OK with you and with @potiuk I suggest to split 1-2 to another PR that we can review quickly & separately and afterwards think about 3. WDYT?

I agree with you, let's split 1-2 and then wait for #16915. If @potiuk agrees, I will split and create a new PR.

@potiuk
Copy link
Member

potiuk commented Jul 13, 2021

Sure !

@pmalafosse
Copy link
Contributor

Nice new operator!

There is this issue about TableauRefreshWorkbookOperator operator FYI #16669 @ciancolo

@ciancolo
Copy link
Contributor Author

@pmalafosse I think that that issue is solved (or partially) with PR #16916 with the deprecation of the access token authentication.

@eladkal
Copy link
Contributor

eladkal commented Aug 9, 2021

hi @ciancolo #16915 was just merged.
Please check if this PR is still needed. If so please rebase/fix conflicts and explain what is the functionality added that isn't covered by TableauOperator

@ciancolo
Copy link
Contributor Author

I think #16915 covers the functionality of this PR.

The current PR is not needed anymore.

@eladkal eladkal closed this Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants