Allow all operations except the content indexer to work when suspended. Allow manually resuming file content indexing while on battery. Allow some operations to run in parallel.
BUG: 378597
Diff Detail
Diff Detail
- Repository
- R293 Baloo
- Branch
- master-scheduler (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 7366 Build 7384: arc lint + arc unit
Comment Actions
- Minor simplification + fix: call checkUnindexedFiles when folder watches are installed instead of scheduleIndexing which can't check for changes in indexable files.
Comment Actions
Sounds good to me. I will try to do a proper review as soon as I can. Sorry for the delay.
Comment Actions
If I understand your summary correctly, your patch changes different things. While these are clearly related, can you try to separate the changes?
Comment Actions
Split-up scheduler overhaul patchset.
- The power state signal should only be emitted when the power state changes.
- Re-write the the file index scheduler.
- Update the balooctl tool with the changed suspend/resume behaviour.
- Make use of the scheduler halt method.
- Check for unindexed files when folder watches are installed.
Comment Actions
@smithjd You have probaly become another victim of phabricator/arc. Your commits have been squashed by arc ...
If you wan't your commits to stay separated, you have to do a git checkout of the first commit, do a arc diff HEAD^1, checkout the next commit, arc diff HEAD^1, and so on.
Comment Actions
Commits are squashed on arc land anyway, but the branch can be manually pushed keeping the separate patches. If you mean separated into different reviews, I don't think it's unclear why there are small changes to code related to the scheduler, or why it might be advantageous to separate these into different reviews.
Is there any practical reason why a simple arc land wouldn't suffice here?
Comment Actions
Because pushing independent stuff in a single commit just sucks for the reviewer, and it sucks if you want to understand the git history later.
You are changing 5 different things in your patches. I can not see which changed source line relates to which change.
Comment Actions
According to the arc documentation, a --merge commit should land the commits separately.
Comment Actions
For future reference, the preferred method to implement a multi-commit patch chain is with multiple Phabricator revisions. See https://community.kde.org/Infrastructure/Phabricator#If_the_patches_are_all_for_the_same_project
Comment Actions
- Re-write the the file index scheduler.
- Update the balooctl tool with the changed suspend/resume behaviour.
Comment Actions
This still contains two unrelated changes. Can you please submit each one individually?
Comment Actions
- Re-write the the file index scheduler.
- Update the balooctl tool with the changed suspend/resume behaviour.
Comment Actions
- Re-write the file index scheduler. Combine content indexer suspend logic.
- Update the balooctl tool with the changed suspend/resume behaviour.
- Re-order and use IndexerState to prioritize the indexer thread pool.
- Newline and tab fixes.
- Prevent a scheduler race condition.
- Separate runnableStateChanged into two functors.
- Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
Comment Actions
Split this into independent changes.
Add add comprehensive description to the summary - a bug reference is insufficient.
| src/file/fileindexscheduler.cpp | ||
|---|---|---|
| 102 | You sneak in a unmentioned and significant change - you allow to run several indexer tasks to run in parallel! | |
| 124 | Whats that supposed to do (I have an idea, but thats ugly ...) | |
| src/file/fileindexscheduler.h | ||
| 95 | Make this a QFlags ... | |
Comment Actions
... and thats the problem - this change does 3 different things. Make each one a separate revision.
| src/file/fileindexscheduler.cpp | ||
|---|---|---|
| 124 | This starts a QRunnable at a negative priority (in this case the negative integer value for NewFiles) and then calls runnableStarted(NewFiles) with the enum of the started runnable. | |
| src/file/fileindexscheduler.h | ||
| 95 | There is no cast from int to enum anywhere here. There is a cast fron enum to int, to a negative integer used to set the QRunnable thread priority. | |
Comment Actions
- Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
Avoid setting a priority on the runnables.
Comment Actions
- Re-write the file index scheduler. Combine content indexer suspend logic.
- Update the balooctl tool with the changed suspend/resume behaviour.
- Re-order and use IndexerState to prioritize the indexer thread pool.
- Newline and tab fixes.
- Prevent a scheduler race condition.
- Separate runnableStateChanged into two functors.
- Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
- Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
- Simplify locking the scheduler.
- Adapt index cleaner to scheduler.
Comment Actions
- Re-write the file index scheduler. Combine content indexer suspend logic.
- Update the balooctl tool with the changed suspend/resume behaviour.
- Re-order and use IndexerState to prioritize the indexer thread pool.
- Newline and tab fixes.
- Prevent a scheduler race condition.
- Separate runnableStateChanged into two functors.
- Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
- Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
- Simplify locking the scheduler.
- Adapt index cleaner to scheduler.
Exporting the storage object is not part of this patchset.
Comment Actions
- Re-write the file index scheduler. Combine content indexer suspend logic.
- Update the balooctl tool with the changed suspend/resume behaviour.
- Re-order and use IndexerState to prioritize the indexer thread pool.
- Newline and tab fixes.
- Prevent a scheduler race condition.
- Separate runnableStateChanged into two functors.
- Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
- Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
- Simplify locking the scheduler.
- Adapt index cleaner to scheduler.
- Rebase
Comment Actions
"Locking" the scheduler is simpler here than identifying that it "has gone idle". "Locking" only happens here for a small number of runnables (two) that can't run concurrently.
Comment Actions
If you don't understand the code, do not change it.
This is not about avoiding to be scheduled, but missing the schedule signal.
You are mixing a ton of different things here, clean it up!