-
Notifications
You must be signed in to change notification settings - Fork 16
DM-10345: provide management for long running searches #415
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
Conversation
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.
This is nice! My local deployment showed couple of minor issues:
- When having 2 catalog result, one table is focus, and trying to show it again looks like it triggers search again with spinning wheel but no 'send to background' button
- I've set an email but didn't get it (do i need smtp locally configured?)
- if i abort the job, the server logs says that the search has been cancel but later on the job ends and logs shows that it has downloaded the table effectively on the server: is this on purpose to reuse the search cache?
@@ -123,6 +123,7 @@ function buildTablePart(llApi) { | |||
* @prop {string} tbl_group the group this table belongs to. Defaults to 'main'. | |||
* @prop {number} pageSize the starting page size. Will use the request's pageSize if not given. | |||
* @prop {boolean} removable true if this table can be removed from view. Defaults to true. | |||
* @prop {boolean} backgroundable true if this search can be sent to background. Defaults to false. |
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.
is backgroundable set to be true in default?
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.
Good catch. I made it default to true and did not update this jsdoc.
Review completes. All looks great.
|
'Show results' from background monitor is not backgrounbable by design. It should be fast, since it's already completed.
Email notification is not implemented. It was not a feature prior to conversion. Also, I think it could be annoying if an email is sent on every search.
Yes, we remove the job, but did not kill the task. In most cases, the task is a service call or a database query. There no api to cancel it, yet.
Yes, there is a one second delay by design. |
Is not coming back for me, it gets stuck. Plus, it looks like is redoing the search, why not showing the table as active, why it goes around the flux flow and refresh image, table and chart?
If it's not implemented, please, remove the input field form the UI, is misleading.
Understand. |
https://jira.lsstcorp.org/browse/DM-10345
Normally, when a search is submitted, a table will appears masked with a loading icon.
The masked panel will be replaced with table data when the search results returned.
With this PR, when a search is backgroundable, a 'send to background' button will be added to the bottom of the loading icon. If pressed, it will close the masked table and send the search to the background. The job will appear in the Background Monitor.
Per Xiuqin request, backgroundable is set to true by default.
This feature will be available to all Firefly's derived projects, ie /irsaviewr, /suit.
To test:
http://localhost:8080/firefly/
Also: this PR also fixes https://jira.lsstcorp.org/browse/DM-11252