WIP: Scheduler Stage 3 (Cron support)#487
WIP: Scheduler Stage 3 (Cron support)#487mfix22 wants to merge 7 commits intoplotly:masterfrom jakedex:scheduler-stage-3
Conversation
|
|
||
| import {Row} from '../../layout.jsx'; | ||
|
|
||
| // react-select requires options to have `label` and `value` keys |
There was a problem hiding this comment.
thx a lot for adding this kind of comment!
| onChange={this.handleIntervalChange} | ||
| /> | ||
| </div> | ||
| <Row style={Object.assign({}, rowStyleOverride, { marginTop: '8px', borderTop: '1px solid #c8d4e3', paddingTop: '24px' })}> |
There was a problem hiding this comment.
linter fails here with "maximum line kength exceeded"
app/constants/constants.js
Outdated
| /* eslint-disable no-multi-str */ | ||
| import {concat} from 'ramda'; | ||
|
|
||
| // Default to 1 week for `refreshInterval` for backwards compatability |
There was a problem hiding this comment.
for backwards-compatibility?
There was a problem hiding this comment.
Oh, I've just got it. This is to prevent old versions of falcon from running new scheduled queries as setInterval(runQuery, NaN). Could you add something like that to comment, please?
There was a problem hiding this comment.
I believe this is to address my concerns about what happens if an non-cron-supporting version of Falcon is started in an environment where there are persisted queries that have been created with a cronInterval.
n-riesco
left a comment
n-riesco
left a comment
There was a problem hiding this comment.
Looks very good to me. Thanks for splitting the code into small files, easy to read!
I'm sure the styles are still work in progress. Here's a screen capture with issues I noticed:
app/constants/constants.js
Outdated
| /* eslint-disable no-multi-str */ | ||
| import {concat} from 'ramda'; | ||
|
|
||
| // Default to 1 week for `refreshInterval` for backwards compatability |
There was a problem hiding this comment.
Oh, I've just got it. This is to prevent old versions of falcon from running new scheduled queries as setInterval(runQuery, NaN). Could you add something like that to comment, please?
| onChange={this.updateRefreshInterval} | ||
| /> | ||
| <div style={{width: '65%', minHeight: '108px'}}> | ||
| <CronPicker onChange={this.handleIntervalChange} /> |
There was a problem hiding this comment.
we should set initialModeId here (and be smart if the scheduled query uses the old format).
| @@ -206,34 +207,38 @@ export class PreviewModal extends Component { | |||
| </Row> | |||
| )} | |||
| {this.props.loggedIn && ( | |||
There was a problem hiding this comment.
how about we do something like this and show a LogIn button if needed?
{this.props.loggedIn ? (...edit/delete...) : (...login..)} Fix linting from Prettier
get initial cron mode id from query
Add login button to preview modal
Move getInitialCronMode to cron-helpers
Fix linting from Prettier
get initial cron mode id from query
Add login button to preview modal
Move getInitialCronMode to cron-helpers
| return 'MONTHLY'; | ||
| } | ||
| } else if (refreshInterval) { | ||
| if (refreshInterval <= 60) { |
There was a problem hiding this comment.
refreshInterval <= 60 * (1 + 5) /2
| } else if (refreshInterval) { | ||
| if (refreshInterval <= 60) { | ||
| return 'MINUTE'; | ||
| } else if (refreshInterval <= 5 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 60 * (5 + 60) /2
| return 'MINUTE'; | ||
| } else if (refreshInterval <= 5 * 60) { | ||
| return 'FREQUENTLY'; | ||
| } else if (refreshInterval <= 60 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 60 * 60 * (1 + 24) /2
| return 'FREQUENTLY'; | ||
| } else if (refreshInterval <= 60 * 60) { | ||
| return 'HOURLY'; | ||
| } else if (refreshInterval <= 24 * 60 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 24 * 60 * 60 * (1 + 7) /2
| return 'HOURLY'; | ||
| } else if (refreshInterval <= 24 * 60 * 60) { | ||
| return 'DAILY'; | ||
| } else if (refreshInterval <= 7 * 24 * 60 * 60) { |
There was a problem hiding this comment.
refreshInterval <= 24 * 60 * 60 * (7 + 30) /2
| return 'DAILY'; | ||
| } else if (refreshInterval <= 7 * 24 * 60 * 60) { | ||
| return 'WEEKLY'; | ||
| } else if (refreshInterval <= 30 * 24 * 60 * 60) { |
There was a problem hiding this comment.
The maximum refresh interval is 2147483647/1000 (see MDN)
|
3 participants
Closing in favor of local branch at #488
This is a WIP PR that adds
cronIntervalsupport to the frontend 👍We will shortly add the backend support and merge it in with this branch. We just wanted to get this up for @n-riesco to review before doing so.
TODO: