Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Review: Approve OK, merging it, but there's some outstanding stuff: * Important: the TableLookup in FTS can currently explode. The new schema version needs to add AUTOINCREMENT to the `id' row of all tables in TableLookup. We should do this before releasing a new tarball. * TableLookup.get_value: please prepare the query in the constructor. * Configure isn't checking for xapian being there * Add a flag to disable FTS? (keeping the Xapian dependency avoidable) -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
I like the last one :) On Fri, Feb 10, 2012 at 12:30 PM, Siegfried Gevatter rai...@ubuntu.comwrote: Review: Approve OK, merging it, but there's some outstanding stuff: * Important: the TableLookup in FTS can currently explode. The new schema version needs to add AUTOINCREMENT to the `id' row of all tables in TableLookup. We should do this before releasing a new tarball. * TableLookup.get_value: please prepare the query in the constructor. * Configure isn't checking for xapian being there * Add a flag to disable FTS? (keeping the Xapian dependency avoidable) -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 You are subscribed to branch lp:zeitgeist. -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
1550+void Indexer::DropIndex () Are we not leaking db and enquire in this method? -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Functionally tested in Unity and working well. Unit tests passing. However - There seems to be a fairly bad leak somewhere. Try repeatedly searching for 'u' or something like that and you'll see the memory consumption go up fairly fast. Nope, sorry can't reproduce that, the first search does indeed increase the mem usage considerably, but that is just xapian initializing its caches afaict. If i search for the same thing over and over again the mem usage stays constant here. This need to be Commit() and db-commit(). You should probably also surround it with a try/catch. Fixing... Are we not leaking db and enquire in this method? db is closed and deleted, but yes enquire is leaked. Fixing. -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Functionally tested in Unity and working well. Unit tests passing. However - There seems to be a fairly bad leak somewhere. Try repeatedly searching for 'u' or something like that and you'll see the memory consumption go up fairly fast. Nope, sorry can't reproduce that, the first search does indeed increase the mem usage considerably, but that is just xapian initializing its caches afaict. If i search for the same thing over and over again the mem usage stays constant here. Odd, now I can't reproduce it here either... I swear I had it sitting at around 16mb writable, and while searching I could see it crawl 1mb at a time all the way past 30mb... But now it sits steady at around 14mb writable (which is still surprisingly much, but stable at least). -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Review: Approve Looking good to me. I'd like someone else to +1 it before we merge though... Outstanding work Michal! -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
I have been using it for 2 days now... I noticed an small increase in memory consumption around 2-4 MB However this is nothing that really bothers me AWESOME WORK On Thu, Feb 9, 2012 at 11:47 AM, Mikkel Kamstrup Erlandsen mikkel.kamst...@gmail.com wrote: Review: Approve Looking good to me. I'd like someone else to +1 it before we merge though... Outstanding work Michal! -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 You are subscribed to branch lp:zeitgeist. -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Review: Needs Fixing Awesome! C++ FTS ftw. - Add COPYING.GPL3, otherwise the tarball can't be re-distributed. - Considering sharing a get_flags_for_log_level or even set_log_level function between ZG and FTS? - s/ver != DatabaseSchema.CORE_SCHEMA_VERSION)/ver DatabaseSchema.CORE_SCHEMA_VERSION/ What's the rationale for this? We don't know changes won't break compatibility - Can you explain the // Don't disconnect monitors using service names? I didn't really review the C++ stuff (I'm asuming you and Mikkel reviewed each other's stuff already?). -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Awesome! C++ FTS ftw. - Add COPYING.GPL3, otherwise the tarball can't be re-distributed. On it... - Considering sharing a get_flags_for_log_level or even set_log_level function between ZG and FTS? I don't think that's really necessary, strictly speaking it'd be a utility function for a specific app, and has no place in a library. - s/ver != DatabaseSchema.CORE_SCHEMA_VERSION)/ver DatabaseSchema.CORE_SCHEMA_VERSION/ What's the rationale for this? We don't know changes won't break compatibility Does that mean we should automatically assume that the possible changes do break stuff? This is only used with read-only database so I don't see any harm - either the reading will continue to work or you'll get some run-time errors, I find that better than just not working with even trying. - Can you explain the // Don't disconnect monitors using service names? As said on IRC, it prevents some races by allowing the internal extensions to register a monitor with a service name (races that would otherwise cause missed notifications when the external daemon is starting and didn't have a chance to register a monitor) I didn't really review the C++ stuff (I'm asuming you and Mikkel reviewed each other's stuff already?). Partially, but we have tests, so it has to work, right?! :) -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Review: Needs Fixing Functionally tested in Unity and working well. Unit tests passing. However - There seems to be a fairly bad leak somewhere. Try repeatedly searching for 'u' or something like that and you'll see the memory consumption go up fairly fast. -- https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022 Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist. ___ Mailing list: https://launchpad.net/~zeitgeist Post to : zeitgeist@lists.launchpad.net Unsubscribe : https://launchpad.net/~zeitgeist More help : https://help.launchpad.net/ListHelp
Re: [Zeitgeist] [Merge] lp:~zeitgeist/zeitgeist/fts++ into lp:zeitgeist
Review: Needs Fixing
1583+void Indexer::Flush ()
1584+{
1585+ db-flush ();
1586+}
This need to be Commit() and db-commit(). See
http://xapian.org/docs/apidoc/html/classXapian_1_1WritableDatabase.html#cbea2163142de795024880a7123bc693.
You should probably also surround it with a try/catch.
--
https://code.launchpad.net/~zeitgeist/zeitgeist/fts++/+merge/92022
Your team Zeitgeist Framework Team is subscribed to branch lp:zeitgeist.
___
Mailing list: https://launchpad.net/~zeitgeist
Post to : zeitgeist@lists.launchpad.net
Unsubscribe : https://launchpad.net/~zeitgeist
More help : https://help.launchpad.net/ListHelp
