Re: [Tracker] Request for review: Make it possible to adjust GraphUpdated delay in the options

[Martyn Russell <martyn lanedo com>]

On 02/01/14 12:40, Philip Van Hoof wrote:
> > From 266f4757bbd4e73a38eafd384d268059bf1b7617 Mon Sep 17 00:00:00 2001
> From: Philip Van Hoof<philip codeminded be>
> Date: Sat, 23 Nov 2013 10:28:35 +0100
> Subject: [PATCH] Make it possible to configure the delay for GraphUpdated

Hello Philip,

My review below:

> Helps fixing JB# 11570
> ---
>   .../org.freedesktop.Tracker.Store.gschema.xml.in   |  5 +++
>   src/tracker-store/tracker-config.c                 | 40 ++++++++++++++++++++++
>   src/tracker-store/tracker-config.h                 |  5 +++
>   src/tracker-store/tracker-config.vapi              |  1 +
>   src/tracker-store/tracker-dbus.vala                |  6 ++--
>   src/tracker-store/tracker-main.vala                |  3 +-
>   src/tracker-store/tracker-resources.vala           |  7 ++--
>   7 files changed, 61 insertions(+), 6 deletions(-)
>
> diff --git a/data/gschemas/org.freedesktop.Tracker.Store.gschema.xml.in 
> b/data/gschemas/org.freedesktop.Tracker.Store.gschema.xml.in
> index bd4ac8d..f7c4565 100644
> --- a/data/gschemas/org.freedesktop.Tracker.Store.gschema.xml.in
> +++ b/data/gschemas/org.freedesktop.Tracker.Store.gschema.xml.in
> @@ -24,5 +24,10 @@ Boston, MA  02110-1301, USA.
>         <_summary>Log verbosity</_summary>
>         <_description>Log verbosity.</_description>
>       </key>
> +    <key name="graphupdated-delay" type="i">
> +      <default>1000</default>
> +      <_summary>GraphUpdated delay</_summary>
> +      <_description>Delay in ms. at which GraphUpdated will happen when signalling data is 
> available.</_description>

Can I suggest a more descriptive description :)

"Period in milliseconds between GraphUpdated signals being emitted when 
indexed data has changed inside the database.".

I personally would use "graph-updated-delay", since we use words as 
separations for config keys, etc.

> +
> +gint
> +tracker_config_get_graphupdated_delay (TrackerConfig *config)
> +{
> +       g_return_val_if_fail (TRACKER_IS_CONFIG (config), 0);

This should return the default as per the .xml.in file, i.e. 1000 
ideally. I usually #define the defaults at the top of the file (at least 
for tracker-miner-fs). Would be better if we could just use the xml file 
value of course :)

> +void
> +tracker_config_set_graphupdated_delay (TrackerConfig *config,
> +                                           gint           value)
> +{
> +       g_return_if_fail (TRACKER_IS_CONFIG (config));

Shouldn't we consider some boundaries?

i.e. perhaps -1 = no updates?
i.e. perhaps 50 ms = minimum?
i.e. perhaps 24 hours = maximum?

Then check for those.

The first thing I think when I see these types of configs in other 
projects, is what are the limitations. We should definitely have this 
documented in the .xml.in file if we go with something here too.

> diff --git a/src/tracker-store/tracker-dbus.vala b/src/tracker-store/tracker-dbus.vala
> index 52b0365..96aed5f 100644
> --- a/src/tracker-store/tracker-dbus.vala
> +++ b/src/tracker-store/tracker-dbus.vala
> @@ -34,6 +34,7 @@ public class Tracker.DBus {
>         static uint notifier_id;
>         static Tracker.Backup backup;
>         static uint backup_id;
> +       static Tracker.Config config;
>
>         static bool dbus_register_service (string name) {
>                 message ("Registering D-Bus service...\n  Name:'%s'", name);
> @@ -85,8 +86,9 @@ public class Tracker.DBus {
>                 return true;
>         }
>
> -       public static bool init () {
> +       public static bool init (Tracker.Config config_p) {
>                 /* Don't reinitialize */
> +               config = config_p;
>                 if (connection != null) {
>                         return true;
>                 }
> @@ -187,7 +189,7 @@ public class Tracker.DBus {
>                 statistics_id = register_object (connection, statistics, Tracker.Statistics.PATH);
>
>                 /* Add org.freedesktop.Tracker1.Resources */
> -               resources = new Tracker.Resources (connection);
> +               resources = new Tracker.Resources (connection, config);
>                 if (resources == null) {
>                         critical ("Could not create TrackerResources object to register");
>                         return false;
> diff --git a/src/tracker-store/tracker-main.vala b/src/tracker-store/tracker-main.vala
> index f8af0ac..ee0430d 100644
> --- a/src/tracker-store/tracker-main.vala
> +++ b/src/tracker-store/tracker-main.vala
> @@ -56,6 +56,7 @@ License which can be viewed at:
>         static void sanity_check_option_values (Tracker.Config config) {
>                 message ("General options:");
>                 message ("  Verbosity  ............................  %d", config.verbosity);
> +               message ("  graphupdated-delay ....................  %d", config.graphupdated_delay);

I would fix the camel case here and make it more human readable, i.e. 
"GraphUpdated Delay". Also, this is a store config right, not a general 
config - shouldn't it be below? It doesn't apply to anyone but the 
tracker-store process is what I mean.

>                 message ("Store options:");
>                 message ("  Readonly mode  ........................  %s", readonly_mode ? "yes" : "no");

i.e. here... ^^

The rest looks good Philip ;)
I would commit unless you want review from Jürg.

--
Regards,
Martyn

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