-
Notifications
You must be signed in to change notification settings - Fork 105
Performance #1035
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
base: master
Are you sure you want to change the base?
Performance #1035
Changes from all commits
9ad6855
ece9180
9f73533
6733116
bbcbbbe
861ceb2
aace775
8061dce
6c43176
b39d4ad
b320191
7bdf7a0
26ca032
8d13947
7779ceb
1258caa
41ed2d3
cd207a6
9bf9f05
c5963e4
f52f7bb
cb43e25
f46dde2
9677991
2421ab3
96e0aae
9f45cd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,52 +1,59 @@ | ||
| import { translate as __, sprintf } from 'foremanReact/common/I18n'; | ||
| import { foremanUrl } from 'foremanReact/common/helpers'; | ||
| import { addToast } from 'foremanReact/components/ToastsList'; | ||
| import { APIActions, get } from 'foremanReact/redux/API'; | ||
| import { | ||
| stopInterval, | ||
| withInterval, | ||
| } from 'foremanReact/redux/middlewares/IntervalMiddleware'; | ||
| import { | ||
| CANCEL_JOB, | ||
| CANCEL_RECURRING_LOGIC, | ||
| CHANGE_ENABLED_RECURRING_LOGIC, | ||
| GET_TASK, | ||
| JOB_INVOCATION_KEY, | ||
| UPDATE_JOB, | ||
| STATUS, | ||
| } from './JobInvocationConstants'; | ||
|
|
||
| export const getJobInvocation = url => dispatch => { | ||
| const fetchData = withInterval( | ||
| let pollTimeoutId = null; | ||
|
|
||
| const FINISHED_STATUSES = [STATUS.FAILED, STATUS.SUCCEEDED, STATUS.CANCELLED]; | ||
|
|
||
| export const isJobFinished = statusLabel => | ||
| FINISHED_STATUSES.includes(statusLabel); | ||
|
|
||
| const extractErrorMessage = response => | ||
| // eslint-disable-next-line camelcase | ||
| response?.data?.error?.full_messages?.[0] || | ||
| response?.data?.error?.message || | ||
| 'Unknown error.'; | ||
|
|
||
| const fetchJobInvocation = (dispatch, url, params = {}) => { | ||
| dispatch( | ||
| get({ | ||
| key: JOB_INVOCATION_KEY, | ||
| params: { include_permissions: true, include_hosts: false }, | ||
| params: { include_hosts: false, ...params }, | ||
| url, | ||
| handleSuccess: ({ data }) => { | ||
| if (!isJobFinished(data.status_label)) { | ||
| pollTimeoutId = setTimeout( | ||
| () => fetchJobInvocation(dispatch, url), | ||
| 5000 | ||
| ); | ||
| } else { | ||
| pollTimeoutId = null; | ||
| } | ||
| }, | ||
| handleError: () => { | ||
| dispatch(stopInterval(JOB_INVOCATION_KEY)); | ||
| pollTimeoutId = null; | ||
| }, | ||
| errorToast: ({ response }) => | ||
| // eslint-disable-next-line camelcase | ||
| response?.data?.error?.full_messages?.[0] || | ||
| // eslint-disable-next-line camelcase | ||
| response?.data?.error?.full_messages || | ||
| response?.data?.error?.message || | ||
| 'Error', | ||
| }), | ||
| 1000 | ||
| errorToast: ({ response }) => extractErrorMessage(response), | ||
| }) | ||
| ); | ||
| }; | ||
|
|
||
| dispatch(fetchData); | ||
| export const getJobInvocation = url => dispatch => { | ||
| stopJobInvocationPolling(); | ||
| fetchJobInvocation(dispatch, url, { include_permissions: true }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be very careful about calling with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the goals here was to reduce the number of times we re-load rarely changing information we already have.
The assumption was that the permissions shouldn't really change as the job runs, even though technically they can and recalculating those every second is a waste in majority of cases.
Yes, at the same time, it is yet another thing the backend has to compute. With that being said, I don't feel strongly about this. We may find out that the savings coming from this are negligible and then we can very well favor the code simplicity over this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other hand, I assume that when user privileges changes, or user logs out, server won't respond for this recurring api request, so there still will be permissions check on server side. So we can omit it on frontend.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Backend always checks permissions, no matter what.
I was under the impression we load these so that we know which action elements can be shown as active. Yes, we could skip that, make everything appear to be clickable and let the backend reject what the user can't do, but then the user experience wouldn't be all that great.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we will check it in first request, it will render buttons correctly, so there won't be any issue, and if something changes (user log out, change permissions etc.) request will fail and user will be redirected or will have to reload, that will send new first request and receive updated privileges. So this should work fine. |
||
| }; | ||
|
|
||
| export const updateJob = jobId => dispatch => { | ||
| const url = foremanUrl(`/api/job_invocations/${jobId}`); | ||
| dispatch( | ||
| APIActions.get({ | ||
| url, | ||
| key: UPDATE_JOB, | ||
| params: { include_hosts: false }, | ||
| }) | ||
| ); | ||
| export const stopJobInvocationPolling = () => { | ||
| clearTimeout(pollTimeoutId); | ||
| pollTimeoutId = null; | ||
| }; | ||
|
|
||
| export const cancelJob = (jobId, force) => dispatch => { | ||
|
|
@@ -66,13 +73,7 @@ export const cancelJob = (jobId, force) => dispatch => { | |
| APIActions.post({ | ||
| url, | ||
| key: CANCEL_JOB, | ||
| errorToast: ({ response }) => | ||
| errorToast( | ||
| // eslint-disable-next-line camelcase | ||
| response?.data?.error?.full_messages || | ||
| response?.data?.error?.message || | ||
| 'Unknown error.' | ||
| ), | ||
| errorToast: ({ response }) => errorToast(extractErrorMessage(response)), | ||
| handleSuccess: () => { | ||
| dispatch( | ||
| addToast({ | ||
|
|
@@ -81,26 +82,12 @@ export const cancelJob = (jobId, force) => dispatch => { | |
| message: infoToast(), | ||
| }) | ||
| ); | ||
| dispatch(updateJob(jobId)); | ||
| }, | ||
| }) | ||
| ); | ||
| }; | ||
|
|
||
| export const getTask = taskId => dispatch => { | ||
| dispatch( | ||
| get({ | ||
| key: GET_TASK, | ||
| url: `/foreman_tasks/api/tasks/${taskId}`, | ||
| }) | ||
| ); | ||
| }; | ||
|
|
||
| export const enableRecurringLogic = ( | ||
| recurrenceId, | ||
| enabled, | ||
| jobId | ||
| ) => dispatch => { | ||
| export const enableRecurringLogic = (recurrenceId, enabled) => dispatch => { | ||
| const successToast = () => | ||
| enabled | ||
| ? sprintf(__('Recurring logic %s disabled successfully.'), recurrenceId) | ||
|
|
@@ -122,19 +109,12 @@ export const enableRecurringLogic = ( | |
| key: CHANGE_ENABLED_RECURRING_LOGIC, | ||
| params: { recurring_logic: { enabled: !enabled } }, | ||
| successToast, | ||
| errorToast: ({ response }) => | ||
| errorToast( | ||
| // eslint-disable-next-line camelcase | ||
| response?.data?.error?.full_messages || | ||
| response?.data?.error?.message || | ||
| 'Unknown error.' | ||
| ), | ||
| handleSuccess: () => dispatch(updateJob(jobId)), | ||
| errorToast: ({ response }) => errorToast(extractErrorMessage(response)), | ||
| }) | ||
| ); | ||
| }; | ||
|
|
||
| export const cancelRecurringLogic = (recurrenceId, jobId) => dispatch => { | ||
| export const cancelRecurringLogic = recurrenceId => dispatch => { | ||
| const successToast = () => | ||
| sprintf(__('Recurring logic %s cancelled successfully.'), recurrenceId); | ||
| const errorToast = response => | ||
|
|
@@ -148,14 +128,7 @@ export const cancelRecurringLogic = (recurrenceId, jobId) => dispatch => { | |
| url, | ||
| key: CANCEL_RECURRING_LOGIC, | ||
| successToast, | ||
| errorToast: ({ response }) => | ||
| errorToast( | ||
| // eslint-disable-next-line camelcase | ||
| response?.data?.error?.full_messages || | ||
| response?.data?.error?.message || | ||
| 'Unknown error.' | ||
| ), | ||
| handleSuccess: () => dispatch(updateJob(jobId)), | ||
| errorToast: ({ response }) => errorToast(extractErrorMessage(response)), | ||
| }) | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not happy about this at all.
Alternatives would be:
Querying two different controllers on every poll is out of the question.