URL prioritisation, split meta WARCs, and miscellaneous bug fixes

[JustAnotherArchivist]

This PR contains the various things I've implemented and fixed back in January and February on my fork, listed below.

Sorry for the huge PR. I was working based on version 2.0.3 (then only available on FalconK's fork) back then, and separating this into individual PRs for each implemented feature/fixed bug now would be really painful. Let me know if you want me to do it anyway.

• Add URL priorisation: --priority-regex/--priority-domain and the get_priority hook can be used to determine in which order URLs are retrieved.
• Load plugins early again, like in wpull 1.x. Before, plugins were loaded after everything was already initialised, which made it difficult to replace functionality by replacing the factory class map. (Fixes Plugins activated too late #383)
• Rewrite parts of the pipeline code to fix the order of URLs; this wasn't really a problem before, but it messed with the priorisation code.
• Fix --concurrent having no effect (--concurrent doesn't enable concurrency #339)
• Pin html5lib version (fixes ImportError with html5lib 0.99999999 (2016-07-14) #332)
• Add splitting meta WARCs when the data WARC is split using --warc-split-meta
• Fix flattening of consecutive slashes in URLs (URLs with a multiple protocol strings in them will give subsequent protocols only one slash #380)
• Handle backslashes in URLs (fixes Fails to properly parse URLs with backslashes #377)
• Handle invalid dates in last modified date (fixes TypeError: Tuple or struct_time argument required #376)
• Strip tab and newline characters from scraped URLs (fixes sitemap parsing/discovery seems broken for www.zeit.de #355)
• Handle empty port correctly (fixes Unable to parse URL with empty port number #340)

[ivan]

You might have noticed already, but your tests are failing in at least

  File "/home/travis/build/ArchiveTeam/wpull/thematrix/wpull/testing/integration/priorisation_test.py", line 161, in test_app_priority_plugin_get_urls_with_priorities
    self.assertEqual(builder.factory['Statistics'].files, 11)
nose.proxy.AssertionError: 5 != 11

Some other tests are probably failing from the earlier merge. Because it would be very tricky to continue test-driven development with failing tests, they should probably all be fixed (or some skipped?) as part of this PR (or in PR to land before this one if you wish.)

[JustAnotherArchivist]

I completely agree.

I remember some tests failing back in February, but definitely not that many. And yeah, some of the failures definitely come from @falconkirtaran's changes, e.g. the unhashable type: dict ones.

I'll look into it. To keep things separate, I will probably fix only the tests broken by my changes in this PR, then prepare a separate PR for the other broken tests.

[JustAnotherArchivist]

My memory was correct: I did get some test failures back in February, but not all of these. As it turns out, this is caused by a version difference in Tornado: my development venv had Tornado 4.5.1 whereas the Travis CI test used 4.5.3.

The problematic commit is tornadoweb/tornado@84bb2e2 (thanks for finding this, @ivan). It replaces localhost with 127.0.0.1 in the return value of tornado.testing.AsyncHTTPTestCase.get_url, reasoning that due to "apparently a recent macos change, connecting to localhost when the ipv6 port is unbound now incurs a 200ms delay, slowing a full test run down by a factor of 20".

Because wpull treats localhost as a different host than 127.0.0.1, all kinds of things in the test suite break (e.g. links to localhost get skipped, which causes the AssertionErrors that the number of files doesn't match the expected value).

I'll override tornado.testing.AsyncHTTPTestCase.get_url in our subclass to always return a localhost URL. Perhaps we should switch to 127.0.0.1 in the future if that delay mentioned in Tornado's commit message becomes a problem.

[JustAnotherArchivist]

That looks much better. The remaining five errors are due to FalconK's changes.

@ivan, could you review please when you get a chance?

[ivan]

s/priorisation/prioritisation/ or the z equivalent in your commit messages as well

I would recommend hard-wrapping your commit messages in the future because most of the git tools either don't wrap or character-wrap.

[JustAnotherArchivist]

I just realised that my prioritisation code won't work as expected probably. It does assign priorities correctly, but it will still first process all todo URLs and only then go to errors. So if a prioritised URL errors out, it will still only be retried after all the other todos have been processed.

It should probably go after all highest priority URLs first, doing first the todos and then retrying the errors, and only then go to the next lower priority. Implementing this will probaby require some changes in how the pipeline and the database interact though. Currently, the pipeline requests an item with either status todo or error, i.e. the pipeline manages when to switch to retries, so to speak. Now, the database could return NotFound if the highest priority of errors is higher than that of todos, such that the pipeline switches to errors and processes those first. But that's a really awful hack.
Instead, the todo vs. error logic should probably be moved to the database, and the pipeline just retrieves an arbitrary item and schedules it for processing.

This is mostly a note for myself so I remember to look into this.

[JustAnotherArchivist]

s/priorisation/prioritisation/ or the z equivalent in your commit messages as well

Do you want me to edit the commit messages (and thereby rewrite the history of my branch, requiring a force-push) or not?
I guess I'm fine with that, assuming GitHub handles it gracefully. Just want to make sure.

[ivan]

You can edit and force-push. I'm not sure how GitHub handles the review comments, but the PR would require a re-review anyway.

[JustAnotherArchivist]

Here we go... For reference, the old branch is preserved at https://github.com/JustAnotherArchivist/wpull/tree/pr393-old.

[JustAnotherArchivist]

I need to rebase again. Forgot to update the commit IDs in the commit messages.
I also still need to look into #393 (comment).
So not ready for merging yet.

[JustAnotherArchivist]

@ivan, could you re-review please?

[anarcat]

tests are still red though... :/

[JustAnotherArchivist]

@anarcat Yeah, those are due to @falconkirtaran's changes between 2.0.1 and 2.0.3. As mentioned earlier, I only fixed the test failures related to my changes. The remaining ones can be addressed afterwards.

[anarcat]

okay, then maybe that can be banged out in #402

[ivan]

I was waiting on confirmation that the new queries (which now include priority) didn't regress performance (discussed on IRC; becoming slower here would be bad) before reviewing this again, but if someone else wants to review this, that is fine with me :-)

[JustAnotherArchivist]

With the added index and INDEXED BY clause in d8fee1a, performance is somewhat worse than before. There really is no way around a small regression since the index is larger than before, so SQLite has more data to crunch on each check-out.

I tested the performance with a large (~10 GiB) DB that I had from a crashed ArchiveBot job. It has over 50 million URLs with roughly 15 million processed. I did not do extensive tests with other databases, but I think this one should cover a fairly extreme case. The test consists of checking out and back in 1000 entries from the DB. The script can be found here. The same script works with both 2.0.3 and d8fee1a; it uses code inspection to determine the number of arguments to check_out to distinguish between the two.

Averaged across 10 runs each, interleaved, on the same machine: 9.3 seconds for the 2.0.3 code, 12.1 seconds for d8fee1a. In other words, a performance penalty of about 30 %. That's not too surprising given the increase in index size: an index on just the status column covers 11 bytes per row while the new index covers the same 11 bytes for status plus 1 byte for priority (because it's all zero in this test, another extreme case since the index on priority is effectively useless) plus 3 or 4 bytes for id (not sure exactly how SQLite switches between the integer representations; cf. here) = 15-16 bytes in total or about 45 % more...

This is certainly not a negligible performance impact, but I'm not sure there's any way to reduce it. At the same time, I am sure that there are other (and simpler?) ways to improve the database's performance by at least a factor 20 (#427), so I don't think these 30 % matter too much.

I intend to do some more tests with different priority values in the table (instead of everything at zero). I'd expect the performance hit to be less severe then since the new index would be more efficient.

[JustAnotherArchivist]

SQLAlchemy 1.3.0 broke my implementation of the INDEXED BY clause via sqlalchemy/sqlalchemy#4509. So that part will require some more work. Although in the context of #427, getting rid of SQLAlchemy entirely is an option, so I might leave that for later and just limit the SQLAlchemy version to <1.3.0 if nobody objects.