Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Scheduling should also add SQL query to grid metadata #503

Closed
tarzzz opened this issue Jul 27, 2018 · 13 comments · Fixed by #504
Closed

Scheduling should also add SQL query to grid metadata #503

tarzzz opened this issue Jul 27, 2018 · 13 comments · Fixed by #504

Comments

@tarzzz
Copy link
Contributor

tarzzz commented Jul 27, 2018

When the query is scheduled from Chart studio, the SQL query is stored with the grid-metadata, and it appears in the webapp under tab 'SQL Query' . It doesn't appear when the query is scheduled from Falcon:

image

The fix here would be to send the query and the refresh interval as the payload for the new created grid.

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Jul 27, 2018

Argh, that's a good point. Unfortunately we'll probably have to fix this for 2.5, because otherwise the following is possible/likely:

  1. User creates and saves a grid in Chart Studio with SQL_1 which is saved in the metadata
  2. User edits the schedule for the query in Falcon-on-prem, and along the way edits the query itself, so SQL_2 is being run now, but SQL_2 is not sent to the metadata (?).
  3. The query in the metadata is no longer the same as the query that's being run on a schedule.

@briandennis @n-riesco is the above correct?

@n-riesco
Copy link
Contributor

@nicolaskruchten I was under the impression that the UI to schedule queries will be removed from onprem (doesn't that include this tab?)

@nicolaskruchten
Copy link
Contributor

It will, but in 2.6

@n-riesco
Copy link
Contributor

@tarzzz What API does the onprem webapp use to set/update this metadata?

@nicolaskruchten I think fixing this issue requires changes on both sides:

  • the webapp needs to understand the new cronInterval
  • Falcon needs to learn how to set/update this metadata

@nicolaskruchten
Copy link
Contributor

Why does the webapp need to understand the new cronInterval?

@n-riesco
Copy link
Contributor

@nicolaskruchten What data goes into this metadata? From @tarzzz 's comment, it sounds like refreshInterval is one of the fields.

@nicolaskruchten
Copy link
Contributor

Ah, I see what you mean. I think that so long as the SQL Query is correct we can get away with sending the 'nearest' refreshInterval. We don't have the bandwidth to modify the webapp-size code at the moment.

@n-riesco
Copy link
Contributor

@nicolaskruchten I don't think it's possible to define the 'nearest' refreshInterval (what would be the refreshInterval of a cronInterval that runs on Mondays, Tuesdays and Thursdays?).

Currently new scheduled queries (i.e those that set cronInterval) have refreshInterval set to weekly (this was decided as a safe default in case a new queries.yaml was run by an old version of Falcon).

@nicolaskruchten
Copy link
Contributor

We'll have to come up with a 'mostly good' heuristic. It won't be very good for the Mon/Tue/Thu case but we'll live with it.

@n-riesco
Copy link
Contributor

@nicolaskruchten Also note that cronInterval can represent monthly schedules, whereas refreshInterval can't.

@briandennis
Copy link
Contributor

re 'mostly good' heuristic: we need to do something similar when determining which cron mode to show in the UI by default. For reference, the code for that is here

@nicolaskruchten
Copy link
Contributor

OK, so it looks like whenever Falcon saves a query, we'll need to PATCH the grid with some metadata and a refresh_interval here: https://api.plot.ly/v2/grids/#partial_update (the docs don't mention refresh_interval but it's accepted (per @tarzzz)

The metadata needs to match what gets populated by the webapp when it creates grids with corresponding falcon queries, e.g.

{
  "connectorUrl": "https://nicolas-026480bf-62bf-4b19-be6c-3.plotly-connector.com:9495",
  "query": "SELECT * FROM auth_group \nLIMIT 10;",
  "connectionId": "postgres-c959ea39-857f-4774-a43e-2978b3420c53"
}

And refresh_interval will need to be one of the options that the webapp can save: 60, 300, 3600, 86400 or 604800. I vote for whatever interval is less-than-or-equal than the cron interval. So "Mon/Tue/Thu" -> 86400, "every month" -> 604800, "daily at hour x" -> 86400 etc.

The impact of the refresh_interval not being totally correct is fairly minimal: it's used to prepopulate the dropdown in the query UI in the webapp. We have to choose one of the options, and this heuristic seems OK to me. I'm not aware of any other place in the app this interval is used/it being wrong would cause big problems. If anyone knows of one, please mention in Slack :)

@nicolaskruchten
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants