D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko closed this revision.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: kde-frameworks-devel, bruns, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, abrahams

D15983: React to config updates inside indexer

[Nathaniel Graham]

ngraham accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R293 Baloo

BRANCH
  arcpatch-D15983

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: kde-frameworks-devel, bruns, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko added a comment.


  In D15983#434247 , @ngraham wrote:
  
  > Can you rebase this on master again? Sorry for the radio silence. :(
  
  
  Nevermind, I also kinda forgot about this :)
  Seems like this patch still applies to current master:
  
# git rebase master
Current branch arcpatch-D15983 is up to date.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: kde-frameworks-devel, bruns, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, abrahams

D15983: React to config updates inside indexer

[Nathaniel Graham]

ngraham added a comment.


  Can you rebase this on master again? Sorry for the radio silence. :(

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: kde-frameworks-devel, bruns, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko added a comment.


  Ping?

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: kde-frameworks-devel, bruns, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko edited the summary of this revision.
poboiko added reviewers: bruns, ngraham.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks, bruns, ngraham
Cc: kde-frameworks-devel, bruns, gennad, domson, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko updated this revision to Diff 45452.
poboiko added a comment.


  It's a bad idea to removeRecursively starting from root of the tree 
(documentid 0).
  If user has indexed `/home/username` folder, there is also an index entry for 
`/home` (that's how IdTreeDB works).
  However, `/home` should not be indexed, according to checks (because it's not 
in includeFolders, while `/home/username` is)
  This will lead to `removeRecursively("/home")` call, which will wipe index 
for `/home/username` as well.
  
  Instead I propose to do checks based on `includeFolders` and `excludeFolders` 
  (which could have changed if config was changed and some directory was added 
to exclude list)

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15983?vs=45451=45452

BRANCH
  arcpatch-D15983

REVISION DETAIL
  https://phabricator.kde.org/D15983

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h
  src/file/indexcleaner.cpp
  src/file/mainhub.cpp

To: poboiko, #baloo, #frameworks
Cc: kde-frameworks-devel, bruns, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko updated this revision to Diff 45451.
poboiko added a comment.


  Rebase on master

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15983?vs=43446=45451

BRANCH
  arcpatch-D15983

REVISION DETAIL
  https://phabricator.kde.org/D15983

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h
  src/file/indexcleaner.cpp
  src/file/mainhub.cpp

To: poboiko, #baloo, #frameworks
Cc: kde-frameworks-devel, bruns, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko marked 3 inline comments as done.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks
Cc: kde-frameworks-devel, bruns, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko updated this revision to Diff 43446.
poboiko added a comment.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.


  Address raised issues

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15983?vs=42971=43446

BRANCH
  update-config

REVISION DETAIL
  https://phabricator.kde.org/D15983

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h
  src/file/indexcleaner.cpp
  src/file/mainhub.cpp

To: poboiko, #baloo, #frameworks
Cc: kde-frameworks-devel, bruns, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, abrahams

D15983: React to config updates inside indexer

[Stefan Brüns]

bruns added a comment.


  Looks good in general, but have to think a little bit more about it ..

INLINE COMMENTS

> fileindexscheduler.cpp:125
>  
> +// This should be above content indexing, because there can be files 
> that should not be indexed in the DB
> +// (i.e. if config was changed)

Should read "This has to be above ...", also do the line break at ~ 80 chars

> fileindexscheduler.cpp:181
> +// Rescheduling content indexer
> +// (so we won't keep indexing files that should not be, bug 373430)
> +if (m_indexerState == ContentIndexing) {

"Interrupt content indexer, to avoid indexing files that should not be indexed, 
bug 373430)"

> fileindexscheduler.cpp:189
> +removeShouldNotIndex(m_xattrFiles, m_config);
> +checkStaleIndexEntries();
> +checkUnindexedFiles();

replace these two lines by

  m_checkStaleIndexEntries = true;
  m_checkUnindexedFiles = true;
  scheduleIndexing();

REVISION DETAIL
  https://phabricator.kde.org/D15983

To: poboiko, #baloo, #frameworks
Cc: bruns

D15983: React to config updates inside indexer

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, Frameworks.
poboiko requested review of this revision.

REVISION SUMMARY
  In case a config was changed, we need to:
  
  1. Stop the ContentIndexer (bug 373430)
  2. Remove all scheduled indexing of files that should not be indexed
  3. Check for stale index entries. Those includes documents marked for 
indexing, that should be removed
  
  (because of that we need to do it inside `scheduleIndexing` before content 
indexing)
  
  4. Finally, check for unindexed files (if new files were added)
  
  (based on D15939: Perform checks for unindexed files and stale index entries 
on startup ). Also note that `IndexCleaner` 
should clean all files, not just those inside `includeFolders`

TEST PLAN
  Seems to be working. But it would be nice to test it more.
  
  1. Add large folder to config. `baloo_file` start indexing it.
  2. Add it to exclude list. `baloo_file` stops indexing it, and removes 
entries from DB.
  3. Remove it from exclude list. `baloo_file` starts indexing it again.

BRANCH
  update-config (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D15983

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h
  src/file/indexcleaner.cpp
  src/file/mainhub.cpp

To: poboiko, #baloo, #frameworks