Skip to content

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

Merged
merged 2 commits into from
Jul 20, 2017

Conversation

loitly
Copy link
Contributor

@loitly loitly commented Jul 19, 2017

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/

  • Catalogs search with a larger radius
  • While loading, click 'send to background'
  • If button is not clicked, it will load when results returned.

Also: this PR also fixes https://jira.lsstcorp.org/browse/DM-11252

@loitly loitly requested review from ejoliet and cwang2016 July 19, 2017 20:26
Copy link
Contributor

@ejoliet ejoliet left a 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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@cwang2016
Copy link
Contributor

Review completes. All looks great.
Just some questions,

  • should provide some way to determine if 'backgroundable' or not?
  • From my end, 'Send to backgroud' button seems shown a couple of seconds later than the spinning? is that on purpose?
  • similar question as being raised by @ejoliet that 'abort' the request looks like 'pause', not really 'abort' the result, since the table is displayed quickly if I sent the same search again.

@loitly
Copy link
Contributor Author

loitly commented Jul 20, 2017

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

'Show results' from background monitor is not backgrounbable by design. It should be fast, since it's already completed.

I've set an email but didn't get it (do i need smtp locally configured?)

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.

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?

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.

From my end, 'Send to backgroud' button seems shown a couple of seconds later than the spinning? is that on purpose?

Yes, there is a one second delay by design.

@loitly loitly merged commit 990e924 into dev Jul 20, 2017
@loitly loitly deleted the DM-10345_background_searches branch July 20, 2017 00:06
@ejoliet
Copy link
Contributor

ejoliet commented Jul 20, 2017

'Show results' from background monitor is not backgrounbable by design. It should be fast, since it's already completed.

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?

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.

If it's not implemented, please, remove the input field form the UI, is misleading.

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.

Understand.

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.

3 participants