Skip to content

Closes #415: Make data source health monitoring an extension.#444

Merged
emtwo merged 1 commit intomasterfrom
emtwo/heartbeat-extension
Aug 10, 2018
Merged

Closes #415: Make data source health monitoring an extension.#444
emtwo merged 1 commit intomasterfrom
emtwo/heartbeat-extension

Conversation

@emtwo
Copy link
Copy Markdown

@emtwo emtwo commented Jun 27, 2018

This PR requires the datasource extension (mozilla/redash-stmo#5) to be merged and deployed first.

This PR then does 2 things:

  1. Remove the data source health monitoring (c08de97)
  2. Add a general ability to have python extensions that are celery tasks.

Note: Once the extension is deployed, this PR should also be updated to include the new redash-stmo dependency version

@washort washort force-pushed the master branch 2 times, most recently from 42c191e to faf9c84 Compare July 30, 2018 14:52
@emtwo emtwo force-pushed the emtwo/heartbeat-extension branch from 20ae13c to 0401214 Compare August 1, 2018 16:00
@emtwo emtwo force-pushed the emtwo/heartbeat-extension branch from 0401214 to 360f245 Compare August 9, 2018 20:27
@emtwo emtwo changed the title [WIP] Closes #415: Make data source health monitoring an extension. Closes #415: Make data source health monitoring an extension. Aug 9, 2018
@emtwo emtwo requested a review from jezdez August 9, 2018 20:39
Copy link
Copy Markdown

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

rwc+ Nicely done @emtwo!

Comment thread redash/worker.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should be more defensive here in case no other extension has created app.periodic_tasks and use something like:

periodic_tasks = getattr(app, 'periodic_tasks', {})
for params in periodic_tasks:
   # ...

Also, we need to open a PR upstream using this change as a new feature "ability for extensions to add periodic tasks". The code change is small, but will be effective. I'm sure upstream has some use for this as well for their own modifications.

Comment thread redash/worker.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We can just rely on the fact that the app.periodic_tasks dict has the API of "variables to be passed to send.add_periodic_task" and write those three lines as:

sender.add_periodic_task(**params)

That way we stay future-proof in case other parameters are added to add_periodic_task.

Copy link
Copy Markdown
Author

@emtwo emtwo Aug 10, 2018

Choose a reason for hiding this comment

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

Unfortunately the argument names for add_periodic_task() are schedule and sig, not signature so this didn't work when I tried it.

We could either change the extension to ensure it labels the arguments correctly or we can do this:

sender.add_periodic_task(*params.values()) but I think this is more fragile because the ordering of the params.values() array is not guaranteed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, I missed that, we should update the extension to match the argument name then, unpacking the keyword arguments is indeed too fragile.

@emtwo emtwo force-pushed the emtwo/heartbeat-extension branch from 360f245 to 5a6f1d6 Compare August 10, 2018 18:38
@emtwo emtwo merged commit bcd0d20 into master Aug 10, 2018
@jezdez jezdez deleted the emtwo/heartbeat-extension branch August 10, 2018 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants