Re: [Tracker] REVIEW: external-crawler branch

[Martyn Russell <martyn lanedo com>]

On 07/08/14 12:36, Martyn Russell wrote:
> On 06/08/14 22:22, Sam Thursfield wrote:
> > > diff --git a/src/libtracker-miner/tracker-file-system.c
> > > b/src/libtracker-miner/tracker-file-system.c
> > > index 7d9a236..dd81f49 100644
> > > --- a/src/libtracker-miner/tracker-file-system.c
> > > +++ b/src/libtracker-miner/tracker-file-system.c
> > > @@ -356,7 +356,7 @@ file_system_finalize (GObject *object)
> > >                          NULL);
> > >         g_node_destroy (priv->file_tree);
> > >
> > > -       if (!priv->root) {
> > > +       if (priv->root) {
> > >                 g_object_unref (priv->root);
> > >         }
> >
> > Definitely squash this into 'libtracker-miner: FileSystem _new() now
> > takes GFile for root'
>
> Thanks, I will.

Done.

> > > commit 8cdc0188e2c4d341e6e9788370e9381e085ae34b
> > > Author: Martyn Russell <martyn lanedo com>
> > > Date:   Fri May 16 12:11:00 2014 +0100
> > >
> > >     libtracker-miner: Fixed finalization logic for root unref, tests
> > > were failing
> >
> > Squash into 'libtracker-miner: IndexingTree _new() now takes a GFile' !
> >
> > Various commits follow that could also do with being squashed.

Done.

> > > +/**
> > > + * SECTION:tracker-enumerator
> > > + * @short_description: Enumerate URI locations
> > > + * @include: libtracker-miner/miner.h
> > > + *
> > > + * #TrackerEnumerator allows you to operate on a set of #GFiles,
> > > + * returning a #GFileInfo structure for each file enumerated (e.g.
> > > + * tracker_enumerator_get_children() will return a #GSList with the
> > > + * children within a parent directory or structure for the URI.
> > > + *
> > > + * This function is similar to g_file_enumerator_next_files_async(),
> > > + * unlike the g_file_enumerator_next() which will return one #GFile at
> > > + * a time, this will return all children at once.
> >
> > That sounds like it could risk locking up the process, or even the
> > system.
> > Some local filesystems take a LONG time to 'ls' sometimes, and chew up
> > huge
> > amounts of IO while doing so. Is this definitely not going to be a
> > performance
> > regression? What if the enumerator is operating on some crazy web API
> > that
> > lists millions of resources?
>
> Actually, you make a good point here. Let me get back to you on this one.

So now we have:

- TrackerEnumerator (GInterface)
- TrackerDataProvider (GInterface) which provides a TrackerEnumerator

and

- TrackerFileEnumerator (using GIO API)
- TrackerFileDataProvider (using GIO API) implemented as default

I discussed this with Carlos and Philip earlier today, I think we're in 
agreement with the new API:

https://git.gnome.org/browse/tracker/tree/src/libtracker-miner/tracker-enumerator.h?h=external-crawler

and

https://git.gnome.org/browse/tracker/tree/src/libtracker-miner/tracker-data-provider.h?h=external-crawler

> > > commit 410923dfc7aaaff9f862d51092d2209d8dad3e13
> > > Author: Martyn Russell <martyn lanedo com>
> > > Date:   Tue Jul 1 10:42:31 2014 +0100
> > >
> > >     libtracker-miner: Added tracker_indexing_tree_get_master_root()
> > > and set in constructed()
> > >
> > >     This function gets the top level root for all roots, e.g. file:///
> > >
> > >     We also create the root nodes in constructed() not init() because
> > > then the
> > >     root GFile has been set in the properties when the object is
> > > initiated
> >
> > I already find the use of the term "root" and "config_root" in
> > TrackerIndexingTree
> > to be quite confusing. Now we have the concept of the "master root",
> > which is
> > mostly called "root", but is different the other type of "root".
> >
> > I think we need some better words :)
> >
> > "Config roots" should perhaps become "configured index points", and
> > the "master
> > root"/"root" can just be referred to as "root" everywhere.
>
> Right. So let me clear this up:
>
> - master root = top most level root node,
>    e.g. file:///
>
> - config root = a root node from GSettings,
>    e.g. file:///home/martyn/Documents
>
> - root = ANY root :) normally config root, but it can also apply to
>    roots added for devices, which technically are not a config root or a
>    master root ;)

Now added to the tracker_indexing_tree_get_master_root() documentation.

> > > commit 011eccbd624aa494e42482c0d809ec588ee2a671
> > > Author: Martyn Russell <martyn lanedo com>
> > > Date:   Tue Jul 1 10:48:11 2014 +0100
> > >
> > >     libtracker-miner: Create nodes / cahces in constructed() not init()
> >
> > Typo: caches

Fixed thanks.

Thanks again for the review!

--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell