Page MenuHomeSoftware Heritage
Differential D663

indexer: Persist computations every 'write_batch_size' computations
ClosedPublic
Actions

Authored by ardumont on Nov 15 2018, 6:53 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDCIDX08c8079dcd6a: indexer: Use functional approach to result
rDCIDXc5c535754110: indexer: Use the right type information
rDCIDX1e6727b25fc4: indexer: Use protected method names
rDCIDXd16a5bb0baee: d/*: Bump dependency to latest swh-storage
rDCIDXefc795e2d6f8: indexer: Persist computations every 'write_batch_size' computations
Summary

Prior to this commit, the computations are stored in db only when all
computations are done.

Depends on D660

Test Plan

tox

Diff Detail

Repository
rDCIDX Metadata indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Nov 15 2018, 6:53 PM
Harbormaster completed remote builds in B2409: Diff 2080.Nov 15 2018, 6:53 PM
ardumont edited the summary of this revision. (Show Details)Nov 15 2018, 6:54 PM
ardumont added a parent revision: D660: Implement mimetype indexer using range of sha1s.
vlorentz added a comment.Nov 16 2018, 10:14 AM

Nitpick: I would expect a variable named batch_write to be a boolean. Maybe rename it to write_batch_size? (meaning "the size of a write batch")

swh/indexer/indexer.py
470

So, if my understanding is good, this return statement returns None if the for loop did not run, and the last results otherwise. Is that what you mean?

And it looks more complicated than it should, particularly because of the shadowing of results.

vlorentz requested changes to this revision.Nov 16 2018, 10:14 AM
This revision now requires changes to proceed.Nov 16 2018, 10:14 AM
ardumont marked an inline comment as done.Nov 16 2018, 10:53 AM
ardumont added inline comments.
swh/indexer/indexer.py
470

So, if my understanding is good, this return statement returns None if the for loop did not run, and the last results otherwise. Is that what you mean?

Yes. I do not particularly care for the results (i do for the tests here).
The content indexer's goal is to write to db (done by persist_index_computations).
What i am aiming with this that the indexer actually write somewhere even in the case of huge range...

Again, it's interesting it gives the result here for tests purposes only.
In the task, we do nothing with the results except for determining the status 'eventful' or not.

And it looks more complicated than it should, particularly because of the shadowing of results.

I might be able to drop the initial results. I did not try.

ardumont edited the summary of this revision. (Show Details)Nov 16 2018, 10:57 AM
ardumont marked an inline comment as done.Nov 16 2018, 11:02 AM
ardumont added inline comments.
swh/indexer/indexer.py
470

And it looks more complicated than it should, particularly because of the shadowing of results.

I might be able to drop the initial results. I did not try.

No, it does not. Unbound error.
Also, regarding complicated, the docstring explains the result is partial (at least, it tried to).

ardumont updated this revision to Diff 2086.Nov 16 2018, 11:03 AM

Change option name to write_batch_size

Harbormaster failed remote builds in B2414: Diff 2086!Nov 16 2018, 11:03 AM
swh-public-ci added a subscriber: swh-public-ci.Nov 16 2018, 11:03 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/38/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/38/console

ardumont updated this revision to Diff 2088.Nov 16 2018, 11:07 AM

Rebase

Harbormaster failed remote builds in B2416: Diff 2088!Nov 16 2018, 11:11 AM
swh-public-ci added a comment.Nov 16 2018, 11:11 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/40/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/40/console

ardumont updated this revision to Diff 2090.Nov 16 2018, 12:14 PM

Rebase

Harbormaster failed remote builds in B2418: Diff 2090!Nov 16 2018, 12:18 PM
swh-public-ci added a comment.Nov 16 2018, 12:18 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/42/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/42/console

ardumont added a comment.Nov 16 2018, 12:21 PM

Build has FAILED

because stacked diff.

vlorentz requested changes to this revision.Nov 16 2018, 1:22 PM
vlorentz added inline comments.
swh/indexer/indexer.py
434

I only just realized that if the caller of index_contents does not consume its values, the yield will block. You should either fix that or mark index_contents as protected method (ie. add a _ prefix)

470

Then remove the return, and set self.results instead? This way there is no return value to serialize and send through Redis.

This revision now requires changes to proceed.Nov 16 2018, 1:22 PM
ardumont marked 2 inline comments as done.Nov 16 2018, 1:45 PM
ardumont added inline comments.
swh/indexer/indexer.py
434

It's a protected method in my mind indeed.

470

Then remove the return, and set self.results instead?

Not functional, i don't like that kind of stuff.
Especially for tests.
(I did not like my partial result as well but did not see better at the time so thx).

This way there is no return value to serialize and send through Redis.

Discussion took place on irc.

The gist of it is:

  • returns a boolean (True: data indexed, False: no data indexed)
  • test the data when testing index_contents instead.
ardumont updated this revision to Diff 2095.Nov 16 2018, 2:31 PM

Adapt according to discussion

  • d/*: Bump dependency to latest swh-storage
  • indexer: Use protected method names
  • indexer: Use functional approach to result
  • indexer: Use the right type information
ardumont retitled this revision from indexer: Persist computations every 'batch_write' computations to indexer: Persist computations every 'write_batch_size' computations.Nov 16 2018, 2:31 PM
Harbormaster failed remote builds in B2423: Diff 2095!Nov 16 2018, 2:31 PM
swh-public-ci added a comment.Nov 16 2018, 2:31 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/43/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/43/console

vlorentz accepted this revision.Nov 16 2018, 2:35 PM
This revision is now accepted and ready to land.Nov 16 2018, 2:35 PM
ardumont marked an inline comment as done.Nov 16 2018, 2:36 PM
ardumont added inline comments.
swh/indexer/indexer.py
470

This way there is no return value to serialize and send through Redis.

Forgot to reply.
There is no serialization (and no redis? -> celery/rabbitmq).
The task (the one spawning the mimetype indexer here) only checks if there is data returned.
If there is, it returns a status eventful, uneventful (as a dict) otherwise ;)
And that's what is serialized.

That's why the boolean is enough.

Closed by commit rDCIDXefc795e2d6f8: indexer: Persist computations every 'write_batch_size' computations (authored by ardumont). · Explain WhyNov 16 2018, 2:39 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
indexer/
51 lines
1 line
tests/
1 line

Diff 2098

View Options

swh/indexer/indexer.py

Loading...
View Options

swh/indexer/mimetype.py

Loading...
View Options

swh/indexer/tests/test_mimetype.py

Loading...
About · Developer information · Legal