rename and export newBatchTime signal in filecontentindexer
ClosedPublic

Authored by astippich on Apr 8 2020, 4:58 PM.

Details

Summary

Applications can subscribe for new files being indexed
by baloo via the dbus interface of fileindexer. However,
currently only the start and finishedIndexing signals are
exported. The latter does only indicate that the file has
been indexed, but not yet committed to the database.
Rename the newBatchTime to better reflect that the
batch has been committed to the database and export
it so that it is possible to know when the transaction
has been committed and thus is safe to query the
database for metadata.

Diff Detail

Repository
R293 Baloo
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Apr 8 2020, 4:58 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptApr 8 2020, 4:58 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Apr 8 2020, 4:58 PM

This is the reason why it is so slow in Elisa?

bruns added a comment.Apr 8 2020, 5:42 PM

In general fine for me.

How will Elisa deal with the indexer maybe crashing inbetween? Do we also need a signal for a batch start?

astippich added a comment.EditedApr 8 2020, 6:09 PM

This is the reason why it is so slow in Elisa?

Do you mean slow for picking up changes in files? Then yes. This happens when a track is modified when Elisa is running and listens for finishedIndexingFile signal. It then immediately queries baloo database, which still has the old values.
Only after the done signal the transaction to baloo's database is complete.

In general fine for me.

How will Elisa deal with the indexer maybe crashing inbetween? Do we also need a signal for a batch start?

From my (limited) understanding, the finishedIndexingFile will no be emitted when an indexers crashes, does it? Then I think it is fine without an additional signal as long as the done() signal is still emitted as it is possible to reset the applications' internal list. Or will it retry some files of the current batch?

bruns added a comment.Apr 8 2020, 7:05 PM

In general fine for me.

How will Elisa deal with the indexer maybe crashing inbetween? Do we also need a signal for a batch start?

From my (limited) understanding, the finishedIndexingFile will no be emitted when an indexers crashes, does it? Then I think it is fine without an additional signal as long as the done() signal is still emitted as it is possible to reset the applications' internal list. Or will it retry some files of the current batch?

It will not be emitted for the file it crashed on, but for the earlier files. So you may have the following for the files 'a, b, c, d':

  • S:a, F:a
  • S:b, F:b
  • S:c <crash>
  • S:a, F:a
  • S:b, F:b
  • done
  • S:c, <crash>
  • S:d, F:d
  • done

So when you only listen to finished and done, you should keep the files in a set, but after the done the earlier files should be committed.

Then it's fine from my application point of view. I can add a corresponding start signal for convenience, though. Your call.

bruns added a comment.Apr 9 2020, 4:27 PM

Then it's fine from my application point of view. I can add a corresponding start signal for convenience, though. Your call.

Lets keep the API as small as possible ...

bruns added a comment.Apr 9 2020, 4:29 PM

Regarding API, done is a little bit vague.

Can you rename it to finishedBatch (or come up with a better name).

How about committedBatch so that it's clear that they have been committed to the database?

astippich updated this revision to Diff 79751.Apr 10 2020, 7:59 AM
  • rename signal
astippich retitled this revision from export done signal in filecontentindexer to rename and export done signal in filecontentindexer.Apr 10 2020, 8:00 AM
bruns requested changes to this revision.Apr 10 2020, 9:19 AM
bruns added inline comments.
src/file/filecontentindexer.cpp
111

Sorry, seems I got a little bit confused, the commitedBatch has to go here.

And when I look at it, you can piggy-back on newBatchTime, which is emitted on each batch. For your use case, just ignore the values.

So new proposal, rename newBatchTime(elapsed, batchSize) to committedBatch(elapsed, batchSize) and export it. Keep done as is.

114

This has to stay as is, as it means everythingdone

This revision now requires changes to proceed.Apr 10 2020, 9:19 AM
astippich updated this revision to Diff 79904.Apr 12 2020, 8:14 AM
  • implement feedback
astippich marked 2 inline comments as done.Apr 12 2020, 8:15 AM
bruns accepted this revision.Apr 12 2020, 8:16 AM

thx

This revision is now accepted and ready to land.Apr 12 2020, 8:16 AM
astippich retitled this revision from rename and export done signal in filecontentindexer to rename and export newBatchTime signal in filecontentindexer.Apr 12 2020, 8:17 AM
astippich edited the summary of this revision. (Show Details)

Thanks

This revision was automatically updated to reflect the committed changes.