469051 - Gloda indexing progress should be shown on Activity Manager window

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Emre Birol]

This operation should be represented to the user in form of activity manager processes and events. Each process should be grouped by folder.

Comment 1

[Emre Birol]

Attachment: Take 1

Context issue needs to be solved. Work in progress.

Comment 2

[David Ascher (:davida)]

wanted, not blocking.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

We need this before we can pref gloda on; we need to know what gloda is up to.  This patch will likely require, at the minimum, that folder traversal indexing utilize 2 passes.  The first pass will count how much work there is for gloda to do, the second pass will actually start doing it.  In the case of folders with ridiculous numbers of messages, we can probably just stop at some number of messages and say "over N messages to go".  Alternatively, if a strategy based mechanism is adopted with persistent (if optimistic) knowledge of the state of indexing, the information will already be roughly in the can and we can just pull from there.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress: implements two-pass indexing

I'm taking a look at this to see if I can help out with a patch.  First step: implement the two-pass indexing approach described in comment 3.  Andrew: is this about what you were suggesting?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: two-pass indexing patch v1

And here's a tested version with one bug fixed.  Perhaps it would be better to review and commit this separately from the second set of changes that will display status in the activity manager, so I'm requesting review for this now.

Note: this patch converts GLODA_MESSAGE_ID_PROPERTY and GLODA_DIRTY_PROPERTY from properties of the GlodaIndexer object to global constants.  I think they make more sense that way, since they don't get used outside the module (so there's no need to export them as part of GlodaIndexer), but they do get used inside the module from outside the GlodaIndexer object in a variety of places.

And if you really need them inside GlodaIndexer at some later date, there's no need to convert them back; just set properties of the object to their constant values, i.e.:

  GLODA_MESSAGE_ID_PROPERTY: GLODA_MESSAGE_ID_PROPERTY,
  GLODA_DIRTY_PROPERTY: GLODA_DIRTY_PROPERTY,

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 375575 [details] [diff] [review]
two-pass indexing patch v1

Wooooooo! A gloda contributor! Wooooooooooooooo!

This is definitely what I was suggesting.

>diff --git a/mailnews/db/gloda/modules/indexer.js b/mailnews/db/gloda/modules/indexer.js
>--- a/mailnews/db/gloda/modules/indexer.js
>+++ b/mailnews/db/gloda/modules/indexer.js
>@@ -1602,56 +1599,84 @@ var GlodaIndexer = {
>+    // Pass 1: count the number of messages to index.
...
>+    for (let msgHdr in this._indexingIterator) {
...
>+      // We used up the iterator, get a new one.
>+      this._indexerGetIterator();
>+    }

You definitely want the call to get a new iterator outside the loop.

I agree with moving the constants to be constants.  Feel free to bring your refactoring eye to the listener stuff too... the only thing that cares right now is mailnews/db/gloda/test/unit/resources/glodaTestHelper.js who implements a listener to know when indexing starts and stops.

Thanks!

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: two-pass indexing patch v2

> You definitely want the call to get a new iterator outside the loop.

D'oh!  Indeed.  Strange that indexing worked when I tested it like this.  I'd think this bug would've caused an infinite loop.  In any case, here's a version of the patch that fixes that problem.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

I've checked in the two-pass indexing patch v2 <http://hg.mozilla.org/comm-central/rev/7991bb9932ca>.  Leaving this bug open for the rest of the fix.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress indexer/activity integration

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #8)
> I've checked in the two-pass indexing patch v2
> <http://hg.mozilla.org/comm-central/rev/7991bb9932ca>.  Leaving this bug open
> for the rest of the fix.

I'd just like to point out bugmail != Standard8. bugmail == asuth. ;-)

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #10)
> I'd just like to point out bugmail != Standard8. bugmail == asuth. ;-)

D'oh!  Sorry about that. :-/

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress indexer/activity integration v2: almost ready

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: indexer/activity integration patch v1

Here's the second part of the work, a patch that displays information about the Gloda indexer's status in the Activity Manager window and the status bar of the main messenger window.

The patch displays the current indexing job in both places, including the folder being indexed, the number of messages being indexed, the number of the current message being indexed, and the percent complete.  It displays completed indexing jobs in the Activity Manager, with the total number of messages indexed and the number of seconds in took to index them.

asuth: let me know if someone else should be reviewing the activity manager-specific code (or just reassign the review).

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: indexer/activity integration patch v2: fixes a few issues

This version fixes a missing string replacement from a recent change in the previous patch, makes string retrieval and formatting more consistent, clarifies the "number of seconds spent indexing" message, reverts glodaIndexerActivity logging to the default configuration, and fixes a variable redeclaration syntax error in log4moz.js.

It also ensures that folder names containing the string "#4" display correctly in the indexingFolderStatusExact status text (where "#4" gets replaced by the percent complete) by reordering the replacements so the folder name replacement happens last.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 376643 [details] [diff] [review]
indexer/activity integration patch v2: fixes a few issues

First, this is great.  It's very exciting to be able to see the indexer doing something with a progress bar on its initial sweep.  Thank you!

The current patch does not capture all of the gloda indexing jobs, which is actually a bit of a blessing.  If we add a call to this._notifyListeners at the end of GlodaIndexer._hireJobWorker (and change the call at the end of GlodaIndexer.workBatch to only call _notifyListeners in the else case against the call to _hireJobWorker), then we get full coverage of all the indexing jobs going on.

Unfortunately, the event-driven indexing jobs have no (single) associated folder so displays "null" as the folder name.  The folder sweep job is even worse; every time the sweep job (provided by GlodaIndexer._worker_indexingSweep) finds a folder that needs indexing, it creates a new folder indexing job (good), but also re-schedules itself to run after that job.  This results in potentially a lot of "null" things being listed.

I think from a UX perspective, the ideal scenario for the initial indexing sweep is that we have a long running "indexing M of N folders" job as well as a "indexing M of N messages in folder X" job happening at the same time.  I feel like we we don't want to actually be logging every event-driven indexing event; maybe some form of aggregation?  That's future work (which you are more than welcome to help with! :) though; I'll ask Bryan.

In any event, my r=asuth request changes are:
1) Make sure _notifyListeners is called for all jobs as per the above.
2) Do not produce events for things that have a null name, but do display them as a process while they're going on so that users know gloda is up to something.
3) (optional) Add a string for "multiple folders" that you can use when the name is 'null'.

I think that should leave us in a very usable state that we can enhance up the wazoo.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: indexer/activity integration patch v3: addresses review comments

> I feel like we we don't want to actually be logging every event-driven
> indexing event;

Yeah, I'm not sure we should be logging folder indexing events in which no messages get indexed either, although these patches do log such events.


> 1) Make sure _notifyListeners is called for all jobs as per the above.

I've done this, but I'm not sure how to test it, so let me know if I've got it right.

Note: this does seem to cause the indexer to notify the activity listener about folder-specific indexing jobs before it has actually determined the folder being indexed, which means that we don't have the name of the folder when constructing the display text in glodaIndexerActivity::onJobBegun, so the activity process now always says "Indexing messages" instead of "Indexing messages in [folder]," although the status text includes the folder name once it becomes available.


> 2) Do not produce events for things that have a null name, but do display them
> as a process while they're going on so that users know gloda is up to
> something.

Ok, done.


> 3) (optional) Add a string for "multiple folders" that you can use when the
> name is 'null'.

Yup, I've added indexing, indexingStatusVague, and indexingStatusExact as non-folder-specific versions of indexingFolder, indexingFolderStatusVague, and indexingFolderStatusExact.

I would have named them indexingFolders, indexingFoldersStatusVague, and indexingFoldersStatusExact, but I thought those would be confusingly similar to the folder-specific names.

Comment 17

[Mark Banner (:standard8)]

+    process.iconClass   = "indexMail";

You probably want to set this on the event as well.

Comment 18

[David :Bienvenu]

Comment on attachment 376871 [details] [diff] [review]
indexer/activity integration patch v3: addresses review comments

+      // XXX Should we really create an event for jobs that don't end up
+      // indexing any items?  Perhaps it's not worth notifying users that we
+      // "indexed 0 messages" in a particular folder.  On the other hand,

It'll be useful for me because then I can blame, er rule out gloda as the cause of all my db's getting opened at once.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset http://hg.mozilla.org/comm-central/rev/24debdbc4e29.