Is this comment still valid?

How about replacing the try...except block with

scenario = message['msg']['task'].get('scnenario')

?
(Assuming that both message['msg'] and message['msg']['task'] is expected to be present at this point.)

Would be nice to have some docstrings for this :)

see: https://docs.pagure.org/greenwave/dev-guide.html#code-style

Nitpick: change to '[...] absence of a result first'

Yes, it's still valid. I'll need to figure out how to test this.

I don't know if task is always part of the message. So it would need to look like this:

scenario = message['msg'].get('task', {}).get('scnenario')

Looking at how testcase is told a few lines above, could this be also message['msg']['testcase']['scenario'], according to the new format?

Also: yep, if task is not guaranteed, then this can get ugly - I would still recommend using this variant, as it's explicit about what can go missing. (A try... except block would also hide if msg is missing from message, which I think should be a case to raise an error.)

This stateful object feels weird to me.

It is not a Results object. It is a ResultsRetriever object, right? It hasn't actually retrieved anything at this point.

And here, this might return [] if results.retrieve() isn't called first, which doesn't happen in this module.

So, .. early you instantiate this empty Results object.. and it goes along having retrieve called on it in multiple times, in multiple ways. Every time it is called, it remembers what it returned so that at the end of the http request, it can return all of them with results.results().

TBH, the description on the PR is exciting. The implementation is overwhelming. Any thoughts on my naming comments, etc above @lholecek?

TBH, the description on the PR is exciting. The implementation is overwhelming. Any thoughts on my naming comments, etc above @lholecek?

I spent a lot of time fixing the code because caching didn't work properly ... and at some point I ended up with badly named classes and bad separation of concerns.

The basic idea is to have small requests so resultsdb can quickly return single result and greenwave can use more precise caching, so that when there is new test result in resultdb or a result is waived, only small part of cache is invalidated. On the other hand this can lead to many more resultdb requests but no unneeded data is fetched (it requests the latest result for given subject and test case name).

If all this sounds OK, I will refactor the code - i.e. fix the classes (naming/separation) add docstrings and perhaps move the resultdb stuff to separate file.

How do we know this is going to perform better (or be kinder to Resultsdb, or less likely to hit timeouts) in Fedora prod?

How do we know that making more, smaller requests to Resultsdb will help it rather than break it worse?

What I am missing here is some analysis about why we have problems now, where the bottlenecks are exactly, and how this is going to help.

:+1: to Dan's concern.

@lholecek do you think you can get a reasonable test/comparison out of a local copy of prod resultsdb?

If not, we can try tagging a version of this into Fedora staging before merging it to see what kind of performance we get out of that. That feels like a good bit more work to pull off, so - a local test/comparison would be preferable.

Here are some performance measurements.

Greenwave and WaiverDB are running locally (no caching), ResultsDB on localhost or staging. Asking for decision for "FEDORA-2018-5a6cc3f4a2" Bodhi update. Tested with:

time http --timeout=99 :5005/api/v1.0/decision \
    <<< '{"product_version": "fedora-28", "decision_context": "bodhi_update_push_stable", "subject_type": "bodhi_update", "subject_identifier": "FEDORA-2018-5a6cc3f4a2", "verbose": true}'

Old code:
- localhost: 15s
- staging: 1m 20s

New code:
- localhost: 4s
- staging: 40s
- prod: 40s

With the new code each remote request takes ~200ms with the new code.

With the old code asking for koji_build on staging takes ~1s when asking for koji_build with the limit=1000 argument.

Querying results in DB is relatively fast but serialization to JSON takes ~120ms locally - it needs to make additional joins. I can get it to ~80ms when omitting groups from being serialized.

Updated!

Still need to find out where in the UMB message is "scenario".

Please review the new code.

I've removed "scenario" part from cache keys for results (it's None in most case anyway). This simplifies the caching a bit and I don't have to figure out where the scenario attribute is stored in messages (I couldn't find such messages - maybe it's only pushed directly to ResultsDB and not published on message bus).

(I rebased the code on latest master.)

I would suggest not to call the real cache key building function here in the tests, but rather to build the expected cache key explicitly (as the test is doing now).

Imagine you accidentally introduce a bug which makes results_cache_key() always return 'Krtek'. This test will pass but it will break horribly in production.

Does this introduce a race condition, where a concurrent HTTP request could have pulled this key into the cache in between the point where this function checks for the keys existence, and then deletes it?

Might be simpler to just unconditionally delete the key, and just ignore any KeyError if it does indeed raise one for deleting non-existent keys.

Is this limiting the request to only one result per page? Why? It seems like it would be more efficient to raise this to the highest value Resultsdb will allow, to avoid doing too many HTTP roundtrips.

Seems a little confusing that this variable is named results but it's actually a ResultsRetriever and not a list of results...

Fixed.

Fixed.

Does this introduce a race condition, where a concurrent HTTP request could have pulled this key into the cache in between the point where this function checks for the keys existence, and then deletes it?

Nice catch. Fixed.

Is this limiting the request to only one result per page? Why? It seems like it would be more efficient to raise this to the highest value Resultsdb will allow, to avoid doing too many HTTP roundtrips.

Increasing the limit makes the request take too long. In most cases only the first (latest) result is needed. For composes there are more requests but the performance is good anyway.

I resolved merge conflicts again.

Ha, Lukas, you have a baby pylint warning causing jenkins to fail. :)

• Actually, that pylint error is just a warning and I'm not even sure why we're running pylint in addition to flake8. Flake8 should be enough. I'll modify the Jenkins job to jettison pylint.
• "FAIL Required test coverage of 55% not reached. Total coverage: 54.65%" - I am okay with adjusting the required coverage down one percent for this in .coveragerc.
• The test suite otherwise passes for me with "98 passed".

OK, code looks much better after the refactor and rebase. It reads and makes sense. Notably, there are only minor changes to the functional tests, which inspires confidence.

See also #279. @lholecek, maybe merge that first, then rebase this on that and then :+1: to go ahead and merge this without further review.

Actually, that pylint error is just a warning and I'm not even sure why we're running pylint in addition to flake8. Flake8 should be enough. I'll modify the Jenkins job to jettison pylint.

This is a common misconception... I remember discussing it with jcline back at the beginning... pylint is actually way better at finding coding errors, like when you have mistyped a variable or argument. It tries to do a lot of type analysis and code flow analysis. flake8 does very little of that, it's really just there to enforce pep8 although it does know how to spot some problems like wrong imports.

Pylint with warnings turned off (which we did in both waiverdb and greenwave) should be very low on false positives and has saved my hide many times when it finds mistakes in code paths not covered by tests. I would recommend not to turn it off.

(Having said that I don't know what pylint error Lukas hit and how irritating it might have been.)

The error was an unused argument in test_policies.DummyCache.get. It seems pylint complains about it, b/c the function has a return None.

As the class is a dummy one anyways, I think it would be ok, to just have pass there. This would actually better express the intention of that method, which would still return None, but without pylint complaining about it.

I think that specific pylint error already helped me found one issue in past. Actually it helps here too, I can use Mock object instead of having this dummy class.

I'll try to cover more of the new code - the coverage is pretty low already.

The coverage of greenwave/tests is now at 55.05% so it's barely passing.

Commit b1104790 fixes this pull-request