mff-uk / odcs

ODCleanStore
1 stars 11 forks source link

Polling for refresh causes tables in refreshing to ignore users request. #478

Closed bogo777 closed 11 years ago

bogo777 commented 11 years ago

With the new way of refreshing, this issue occured. When in Execution monitor, and refresh is underway -> the wheel of Vaadin in the top right corner is turning, when user tries to select execution in table, it is ignored and table is only refreshed instead.

This is unpleasant and unnecessary, especially in cases, when no execution is running. I would propose refreshing execution monitor only when some change in execution occurs, which we can easily find out in backend, as it is backend what is doing these changes.

So upon change in execution, backend would inform frontend of the change and the execution monitor would be refreshed.

janvojt commented 11 years ago

So upon change in execution, backend would inform frontend of the change and the execution monitor would be refreshed.

I do not think this is necessary. How would you even implelent it, send an event everytime somthing is logged? That would be extremely performance intensive.

Please correct me if I am wrong, but isn't the whole problem caused by the fact, that we are not using lazy container for loading log messages? From a brief look at the code, it seems like we are loading ALL logs for given execution. Instead, we should use a paginator and load only those few logs that will really be displayed on the screen.

tomas-knap commented 11 years ago

Bohuslav, I thought we are using lazy container for logs?

tomas-knap commented 11 years ago

Regarding emitting events everytime when certain events occur, that could be done for events, but for every single log message, this is too heavyweight. Bohuslav, will the lazy container solve the issue?

tomas-knap commented 11 years ago

Kdyz se refreshuje monitor, tak co vse se aktualne stahuje z db?

bogo777 commented 11 years ago

We are using IntlibLazyQueryContainer, which uses lazy-loading for both logs and execution monitor table. When we weren't using this, loading logs table with plenty(50-100 not thousands as Jakub has in buyer-profiles) pages took minutes.

When refreshing monitor, current page of records should be downloaded every time. There is also query for total count of records, to determine total number of pages. And query for records may be complicated, because there is also loaded pipeline for every records, as we need it to show pipeline name. There could be some errors in settings of container regarding this.

I did not study how the query looks, I assumed reasonable implementation.

For emitting events from backend, I meant only event when pipeline status -> PipelineExecutionStatus changes. And it would be signal for refreshing execution monitor table -> main table on execution monitor tab. For refreshing DebuggingView -> right part of execution monitor window, I assumed, that there will always be changes for running execution in 3 or 5 seconds and it would be refreshed in that interval.

bogo777 commented 11 years ago

This is suggested solution from Jakub: I suggest you use my remote testing database, generate 5000 entries to the table storing the execution monitor results (just copy one of the records) and then implement it so that it is fast. You don't need to refresh by downloading everything all the time. Just check with a single simple query whether there is a record newer than the last one you are displaying and if yes, download only those.

This exact practise is not possible on current container, due to its query nature, but the idea with "light" query for detecting change could be used. This would require "last_change" column on pipeline execution records which would be updated every time this record is changed in database.

tomas-knap commented 11 years ago

Bohuslav, so you suggest:

  1. add column lastUpdated to the execution records.
  2. Every 5s check which execution records were updated based on the locally cached last update timestamp (the same for all execution records). Then only the updated records (those with lastUpdated > timestamp cached in frontend) are fetched from db, and put to the table, lastUpdated column is updated for these records.
  3. refreshing of events/logs for one particular opened execution record will behave as now
janvojt commented 11 years ago
  1. We already have column timestamp in the table LOGGING_EVENT. We could use this.
janvojt commented 11 years ago

BTW looking at the database schema, I realized we do not have any indexes on tables. I need to add them, this might also be a part of the problem.

tomas-knap commented 11 years ago

regarding the column timestamp, I would suggest to have a separated column for the records in the execution monitor. Because the the particular events are not used when deciding whether the execution monitor should be refreshed, the refreshing of the records in exec monitor are based on the change in status - schedulled, running, finished,..

bogo777 commented 11 years ago

This occured to me: Vidim tam pak ale jeste ten problem s tim dotazem, tedy ze by to melo volat jedno pro vsechny nebo Nkrat pro jeden record.

Isn't this how hibernate works in getResultList()? This method is called in IntlibQuery, but records are fetched one by one. And in schedules, where different container and facade method for getting schedules is called, records are also fetched one by one? At least I think so from the log...

bogo777 commented 11 years ago

Ok, I implemented first version of agreed solution, and it works for me, please test it.

janvojt commented 11 years ago

Isn't this how hibernate works in getResultList()? This method is called in IntlibQuery, but records are fetched one by one. And in schedules, where different container and facade method for getting schedules is called, records are also fetched one by one? At least I think so from the log...

Definitely not. Hibernate should execute only a single query. Well actually 2 queries should be executed by vaadin container - 1 for getting the count of all records matching filter, and 1 for fetching the records for currently viewed page.

tomas-knap commented 11 years ago

Od Bohuslava: V refreshi jsem udělal to, na čem jsme se domluvili. Přidal jsem trigger na update v tabulce exec_pipeline, který nastaví lastChange u záznamu. V execution monitoru si pamatuju čas posledního refreshe a každých 5 vteřin ho porovnám s MAX(lastChange) v db. Refresh provedu jen pokud je tam novější záznam. V tom případě updatnu i čas posledního refreshe.