Diff Detail
- Repository
- R293 Baloo
- Branch
- master-filedeindexer (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 2216 Build 2234: arc lint + arc unit
Under the premise that I still have to learn about baloo's inner workings, here are some concerns:
- I'm not convinced, that index cleaning should be part of the dbus interface. Why should cleaning be done out of process? Do we really want any application to trigger database manipulation? Could you elaborate your rationale please?
- Which application is using baloo dbus interface anyway and which functions? @mgallien How is elisa using it?
- Purging stale entries is by far not enough to get the index in a good shape. As soon as removable drives or network shares are involved all sorts of weird things can happen. IndexCleaner is much too simple to account for that.
- Looks like indexcleaner has been dead code until now. Do we need to be concerned about Vishesh's commit message? https://cgit.kde.org/baloo.git/commit/?id=ea2afe88b0c4299d7540e5b6c8b8e46858336f0c
src/file/fileindexscheduler.cpp | ||
---|---|---|
166 | Same as above | |
233 | Same as above | |
235 | Same as above | |
src/file/fileindexscheduler.h | ||
89 | Nitpick: We're removing db entries not files. Also every file is deindexable. Maybe 'purgeStaleEntries', 'removeLostEntries' or so would be a better name to describe what's going on. | |
114 | Same as above | |
src/file/indexcleaner.cpp | ||
44–45 | Unrelated whitespace change | |
81 | You can use QStringLiteral("%1/").arg(device.mountPath()) here and maybe define it outside the loop | |
src/tools/balooctl/main.cpp | ||
85 | Something like "Update the index. Remove stale entries and add new files." would describe the actions taken more clearly. | |
196 | Do purging before indexing. |
IndexCleaner fits the current implementation of checking for file changes quite well.
- Which application is using baloo dbus interface anyway and which functions? @mgallien How is elisa using it?
- Purging stale entries is by far not enough to get the index in a good shape. As soon as removable drives or network shares are involved all sorts of weird things can happen. IndexCleaner is much too simple to account for that.
IndexCleaner is simple and can handle 99.9% of file deindexing requirements, and is now safe for use in combination with unmounted indexed volumes. IndexCleaner is a vital MIA part of Baloo. An additional purger could conceivably be useful in certain cicumstances, but given that the index is regeneratable data, I don't know that it's even warranted. This could be a good use for baloodb, leaving the generic automatic part to Baloo.
- Looks like indexcleaner has been dead code until now. Do we need to be concerned about Vishesh's commit message? https://cgit.kde.org/baloo.git/commit/?id=ea2afe88b0c4299d7540e5b6c8b8e46858336f0c
As above, this already neatly fits into the workflow.
- Review changes.
src/file/fileindexscheduler.h | ||
---|---|---|
89 | This is doing the opposite of indexing new files, which is removing unfound files from the index. I think deindex is an appropriate term for this. | |
src/tools/balooctl/main.cpp | ||
85 | This is probably already clear enough. check isn't designed to be used all that often; it's already run at kde startup. |
- Optimize and make the loop actually work.
- Change check command description.
- Review changes.
- Fix a threading issue.
- Fix check operation order.
- Use a single StorageDevices object.
src/file/fileindexerconfig.cpp | ||
---|---|---|
63 ↗ | (On Diff #30589) | Looks like you're reverting https://cgit.kde.org/baloo.git/commit/?id=76c207a141423102ab0c5fba43fdafe3e205cca5 with this |
src/file/fileindexerconfig.cpp | ||
---|---|---|
63 ↗ | (On Diff #30589) | No, making the StorageDevices object static so it can be exported. |
- Optimize and make the loop actually work.
- Change check command description.
- Review changes.
- Fix a threading issue.
- Fix check operation order.
- Use a single StorageDevices object.
- Partially revert the last diff, restoring StorageDevices creation on demand.
src/file/fileindexerconfig.cpp | ||
---|---|---|
63 ↗ | (On Diff #30589) | I had presumed that the optimization came from re-using the already existing cache instead of recreating it, rather than creating the SolidDevices object on demand. |
I'm still not convinced this is a sustainable approach.
- The user has no control over the process
- Symlinks pointing to unmounted devices will be removed
- Database management is partially exposed on dbus
- Depending on user's setup this covers only ~80% (my system) of inconsistencies
- It is good to have a tool like this for diagnostic purposes, but the question is: Why do stale entries exist in the first place?
- Regenerating data can be very expensive (several hours) see 1)
Because of those concerns I prefer to do cleaning with baloodb (D11753) which is designed to give the user maximal control.
I'm not requesting changes because I don't want to stand in the way. Others have to decide on this diff.
- New DBus method updateIndex().
- Document indexing options.
- Prevent a scheduler race condition.
- Run de-indexing file check in parallel.
- Rebase for separate statechanged functors.
src/file/indexcleaner.cpp | ||
---|---|---|
76 ↗ | (On Diff #40346) | This example will also detach with or without a const intermediate.
|
82 ↗ | (On Diff #40346) | Same as above. |
src/file/indexcleaner.cpp | ||
---|---|---|
76 ↗ | (On Diff #40346) | No, if done correctly, it will not detach. Try to understand the linked article. |
- New DBus method updateIndex().
- Document indexing options.
- Prevent a scheduler race condition.
- Rebase for separate statechanged functors.
Re-base. Drops setting a runnable priority.