D18890: Add button to reset index database and repair Baloo crashing

[Nathaniel Graham]

ngraham added a comment.


  I'd like to revisit this. Can we get back to it and fix the outstanding 
issues?

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: mart, davidedmundson, bruns, ngraham, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, domson, ashaposhnikov, astippich, spoorun, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, abrahams, 
sebas, apol

D18890: Add button to reset index database and repair Baloo crashing

[Stefan Brüns]

bruns added a comment.


  In D18890#410137 , @davidedmundson 
wrote:
  
  > If we know the DB is corrupted, and it's just a cache, why do we need a 
user facing button?
  
  
  There are many possible ways of corruption:
  
  - non-decodable values
  - entries with dangling parent IDs
  - IDs of no longer existing documents
  - 
  
  These have different reasons, not all reasons are known and understood, not 
all corruptions are critical.
  
  There are issues with races in the file system, where items are 
deleted/created/modified while these are added to the DB. Unfortunately, Qt 
offers no way to handle it, the only save method is to use the 
openat/fstatat/... functions. This is possible to fix, but until then, rinse 
and repeat.
  
  The non-decodable values are hard to handle, this would at least require 
scrubbing the whole DB or building it from scratch. But until it is known what 
causes this corruption, we end up in doing it again and again.
  
  Some of these are quite safe to just ignore, but some parts a lacking sanity 
checks. On the other hand, D12336  is 
waiting for review for 10 months ...

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: mart, davidedmundson, bruns, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, domson, ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, abrahams, sebas, apol

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe added a comment.


  In D18890#419886 , @mart wrote:
  
  > In D18890#410144 , @guoyunhe 
wrote:
  >
  > > In D18890#410137 , 
@davidedmundson wrote:
  > >
  > > > If we know the DB is corrupted, and it's just a cache, why do we need a 
user facing button?
  > >
  > >
  > > It is also possible to move this function to database corruption handler. 
One thing I worry about is: if the database corruption is caused by software 
reason, it will repeat the corrupt-rebuild-corrupt dead loop...
  >
  >
  > maybe also add an heuristic that if has been automatically rebuilt more 
than x times in the last y time, stop doing it?
  
  
  Should work. I will make a patch for Baloo and then update this patch.

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: mart, davidedmundson, bruns, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, domson, ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, abrahams, sebas, apol

D18890: Add button to reset index database and repair Baloo crashing

[Marco Martin]

mart added a comment.


  In D18890#410144 , @guoyunhe wrote:
  
  > In D18890#410137 , 
@davidedmundson wrote:
  >
  > > If we know the DB is corrupted, and it's just a cache, why do we need a 
user facing button?
  >
  >
  > It is also possible to move this function to database corruption handler. 
One thing I worry about is: if the database corruption is caused by software 
reason, it will repeat the corrupt-rebuild-corrupt dead loop...
  
  
  maybe also add an heuristic that if has been automatically rebuilt more than 
x times in the last y time, stop doing it?

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: mart, davidedmundson, bruns, ngraham, plasma-devel, jraleigh, GB_2, 
ragreen, Pitel, domson, ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, abrahams, sebas, apol

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe added a comment.


  In D18890#410137 , @davidedmundson 
wrote:
  
  > If we know the DB is corrupted, and it's just a cache, why do we need a 
user facing button?
  
  
  It is also possible to move this function to database corruption handler. One 
thing I worry about is: if the database corruption is caused by software 
reason, it will repeat the corrupt-rebuild-corrupt dead loop...

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: davidedmundson, bruns, ngraham, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[David Edmundson]

davidedmundson added a comment.


  If we know the DB is corrupted, and it's just a cache, why do we need a user 
facing button?

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: davidedmundson, bruns, ngraham, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe added inline comments.

INLINE COMMENTS

> bruns wrote in kcm.cpp:197
> IMHO this whole block should be a single, blocking command in balooctl, i.e. 
> move the whole logic to balooctl.

Will try.

> bruns wrote in kcm.cpp:210
> Care to elaborate?

If I run "balooctl start" in QProcess, baloo_file always fail to start. I 
didn't figure out the reason. But running baloo_file directly always works.

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: bruns, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> kcm.cpp:197
> +const QString rm = QStandardPaths::findExecutable(QStringLiteral("rm"));
> +const QString balooctl = 
> QStandardPaths::findExecutable(QStringLiteral("balooctl"));
> +const QString baloofile = 
> QStandardPaths::findExecutable(QStringLiteral("baloo_file"));

IMHO this whole block should be a single, blocking command in balooctl, i.e. 
move the whole logic to balooctl.

> kcm.cpp:210
> +
> +// "balooctl start" isn't reliable, so here we run "baloo_file" directly
> +QProcess::startDetached(baloofile, QStringList());

Care to elaborate?

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: bruns, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Nathaniel Graham]

ngraham added a comment.


  No, I don't want a dialog window of any kind. Just a spinner on the page that 
says, "Indexing is in progress" or something was what I had in mind. An inline 
KMessageWidget is okay too. However my major concern was communicating to the 
user that something happened so they don't click the button multiple times.

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: bruns, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Stefan Brüns]

bruns added a comment.


  In D18890#409359 , @ngraham wrote:
  
  > Thanks! This is pretty good as-is, and I can confirm that it works just 
fine. However once the user presses the button, there's no further feedback, 
which could encourage them to repeatedly press it again--not a good idea. Maybe 
while the initial index is being generated, we could display a progress spinner 
(and a label too) and disable the button. What do you think?
  
  
  No, please no progress dialog, a full rebuild can take hours ...

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: bruns, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ashaposhnikov, astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Nathaniel Graham]

ngraham added a comment.


  Thanks!
  
  1. Could we use an inline `KMessageWidget` instead of a dialog box? Dialog 
boxes are annoying.
  2. The text should be something more like "Database is now being rebuilt", 
since if I'm understanding the code, it will be displayed immediately. So it 
isn't actually accurate to say that the database has already been rebuilt.

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe updated this revision to Diff 51436.
guoyunhe added a comment.


  - Make message box translatable

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18890?vs=51435=51436

BRANCH
  master

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

AFFECTED FILES
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe added a comment.


  Screenshot:
  F6608381: image.png 

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe updated this revision to Diff 51435.
guoyunhe added a comment.


  - Add message box for Rebuild Index Database button

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18890?vs=51325=51435

BRANCH
  master

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

AFFECTED FILES
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Nathaniel Graham]

ngraham added a comment.


  Thanks! This is pretty good as-is, and I can confirm that it works just fine. 
However once the user presses the button, there's no further feedback, which 
could encourage them to repeatedly press it again--not a good idea. Maybe while 
the initial index is being generated, we could display a progress spinner (and 
a label too) and disable the button. What do you think?

INLINE COMMENTS

> kcm.cpp:208
> +proc->start(balooctl, args3);
> +proc->waitForFinished();
> +}

This whole section could benefit from a few more line breaks to separate the 
commands into logical groups.

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe added a comment.


  In D18890#409270 , @ngraham wrote:
  
  > Great! Just one more UI suggestion: use the string "Rebuild Index Database"
  
  
  Updated :-)

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe updated this revision to Diff 51325.
guoyunhe added a comment.


  - Reset index database button align right

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18890?vs=51324=51325

BRANCH
  master

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

AFFECTED FILES
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Nathaniel Graham]

ngraham added a comment.


  Great! Just one more UI suggestion: use the string "Rebuild Index Database"

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe added a comment.


  In D18890#409258 , @ngraham wrote:
  
  > I wish this weren't necessary, but I think it's a good idea for the time 
being.
  >
  > Can you reduce the button width so it doesn't span the entire layout? It 
should be as small as possible and right-aligned.
  
  
  I made it right aligned with smaller width.
  
  F6605701: image.png 

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe updated this revision to Diff 51324.
guoyunhe added a comment.


  - Reset index database button align right

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18890?vs=51294=51324

BRANCH
  master

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

AFFECTED FILES
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Nathaniel Graham]

ngraham added a comment.


  I wish this weren't necessary, but I think it's a good idea for the time 
being.
  
  Can you reduce the button width so it doesn't span the entire layout? It 
should be as small as possible and right-aligned.

REPOSITORY
  R119 Plasma Desktop

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

To: guoyunhe, #plasma, #baloo
Cc: ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ashaposhnikov, 
astippich, spoorun, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
abrahams, sebas, apol, mart

D18890: Add button to reset index database and repair Baloo crashing

[Yunhe Guo]

guoyunhe created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
guoyunhe requested review of this revision.

REVISION SUMMARY
  Here are a lot of bug reports of Baloo crashing caused by corrupted
  database. This patch provide a button in KCM to quickly delete and rebuild 
index 
  database.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  kcms/baloo/configwidget.ui
  kcms/baloo/kcm.cpp
  kcms/baloo/kcm.h

To: guoyunhe
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart