Re: [Tracker] Refactoring the filesystem miner

[Carlos Garnacho <carlos lanedo com>]

Hey,

I've been addressing these issues in miner-fs-refactor, more details
below.

On Mon, 2011-11-21 at 15:15 +0000, Martyn Russell wrote:

> 1. Is there something to fix here ?
> +               /* FIXME: Should avoid recursing through children,
> +                * but we're not allowed to modify the tree during
> +                * traversal, nor have a way to skip recursing within
> +                */

This and other remaining FIXMEs have been solved

> 2. I would add some warning here:
> +               /* what are we doing with such file? should happen rarely,
> +                * only with files that we've queried, but we decided not
> +                * to crawl (i.e. embedded root directories, that would
> +                * be processed when that root is being crawled).
> +                */

Done

> 3. We should put all typedefs at the top of the file (or should we) ?
> +typedef struct {
> +       TrackerFileNotifier *notifier;
> +       GNode *cur_parent_node;
> +
> +       /* Canonical copy from priv->file_system */
> +       GFile *cur_parent;
> +} DirectoryCrawledData;

Done

> 4. We are using a lot of sync calls

I guess you mean the g_file_query_info() calls in
file_notifier_add_node_foreach(), I've modified the crawler so it's able
to return GFileInfo, so there's one less stat() per file too.

> 5. Another fixme:
> +               /* FIXME: This supposedly works, but in practice
> +                * won't ever happen as the parent directory
> +                * wasn't being monitored if being ignored
> +                */
>
>
> 6. Another fixme:
> +                                       /* Move only the folder, and
> +                                        * delete all its contents
> +                                        */
> +                                       /* FIXME: it doesn't */

those are gone too :)

> 7. The progress isn't meant to do this (i.e. go to 2%), looks like a 
> regression:
> 21 Nov 2011, 12:04:43:   85%  File System             - Processingâ 03s 
> remaining
> 21 Nov 2011, 12:04:44:   96%  File System             - Processingâ 
> unknown time left
> 21 Nov 2011, 12:04:45:    2%  File System             - Processingâ 
> unknown time left
> 21 Nov 2011, 12:04:45:  â     File System             - Idle

That problem seems to be in master too, I've pushed a fix in the branch

> 8. I think some of the miner-fs logging needs changing, there are some 
> things which are debugging but printed as messages and visa versa:
>
> e.g. these should be debug logs:
> Tracker-Message: Flushing SPARQL buffer, reason: Queue handlers WAIT
> Tracker-Message: (Sparql buffer) Finished array-update with 50 tasks
>
> e.g. these should be messages (i.e. when they're officially in the db, I 
> would skip the "processing" versions here but at least show the 
> "create"/"update"/"delete" messages):
> (tracker-miner-fs:6782): Tracker-DEBUG: Creating new item 
> 'file:///home/martyn/Videos/oceans13.m4v'
> (tracker-miner-fs:6782): Tracker-DEBUG: Creating new item 
> 'file:///home/martyn/Videos/Inception.m4v'

I haven't changed this at the moment, I agree logging may be uncluttered
this way, but this may be done afterwards in master

> 9. I see a critical here when re-running miner-fs after initial index:
> (tracker-miner-fs:6854): GLib-CRITICAL **: g_timer_reset: assertion 
> `timer != NULL' failed

Fixed

> 10. I notice we get events for .xsession-errors now, before we 
> *EXPLICITLY* had code to ignore that file because it just fills logs. We 
> should fix that up:
> (tracker-miner-fs:6854): Tracker-DEBUG: Received monitor event:0 
> (G_FILE_MONITOR_EVENT_CHANGED) for 
> file:'file:///home/martyn/.xsession-errors'

This has been changed to use TrackerIndexingTree to determine early
whether a file is eligible or not, this situation is now fixed.

> 11. Currently make distcheck fails:
> In file included from ../../../src/libtracker-miner/tracker-monitor.h:29:0,
>                   from ../../../src/libtracker-miner/tracker-monitor.c:26:
> ../../../src/libtracker-miner/tracker-indexing-tree.h:30:33: fatal 
> error: tracker-miner-enums.h: No such file or directory

Should be fixed now

> 12. I think we need more tests for:
> â src/libtracker-miner/tracker-indexing-tree.c
> â src/libtracker-miner/tracker-file-notifier.c

Added some tests for TrackerFileNotifier, this could be improved but is
a good start.

> 13. How come the branch adds:
> â tests/libtracker-client/tracker-test

Seems added by accident, removed.

  Carlos