Even faster globs: now with tests#286
Conversation
Validation test + per-bucket glob tests for coiled, ookla, overture, gbif, and blockchain public S3 buckets.
… at bottom All tests now accumulate HTTP logs and assert request counts at the end, matching the pattern from glob_test_ookla.
|
|
||
| # Wire cost assertions — all globs above | ||
| query IIIII | ||
| FROM duckdb_logs_parsed('HTTP') SELECT count(*), sum(request.type == 'HEAD'), sum(request.type == 'GET'), sum(request.type == 'POST'), sum(request.type == 'PUT') GROUP BY context_id ORDER BY context_id; |
There was a problem hiding this comment.
Can we rewrite these to be more robust to changes in logs? This all seems rather brittle / not future proof and will likely require editing in the future.
There was a problem hiding this comment.
I changed the logic to filter only 200 / 202 / 206, that should be robust to retries or other random results.
Yes, this will require changing the numbers if the buckets are updated, but I think that's a trade-off that might be beneficial, in exchange it buys testing on real data.
Would be maybe better to move this tests to a nightly run? So they are not influencing duckdb CI / regular CI on PRs?
I would think there has to be a place for querying actual data, at the cost of flakiness.
There was a problem hiding this comment.
I just dislike hard-coding the number of requests - it's always super flaky, and httpfs tests keep on failing randomly in main for minor changes. Do we need an exact match? Can't we do count < 100 or similar?
There was a problem hiding this comment.
Moving to nightly is a good idea as well, but I would vote for pre-emptively making tests less flaky too. If the GET request count goes from 46 to 47 (likely for external reasons) do we really need this test to fail?
There was a problem hiding this comment.
Also depending on logs in tests is very flaky in my experience because any change to logs (e.g. adding an extra output row) means all these tests need to be rewritten, just like involving this context_id. Can this not be simplified somehow to e.g. look at the MAX request count?
There was a problem hiding this comment.
I am not sure, I think there is value in hard numbers that allows to detect unintended changes of behaviour on our side. I see this can't influence main CI, so that has likely to get require-env that will only be used in httpfs [nightly] CI or for explicit local testing.
Main goal of this was validating the other PR, and that's achieved also without a merge.
Maybe we chat in-office on what's the best path forward.
c51b63a to
28d921a
Compare
2 participants
Builds on top of #284.
It should show that that PR solves most of the issues, and now we either performing the same logic (due to probe reveal few files per folder) OR we have only minor differences, that can either be very positive for recursive/hierarchical listing or minor negative.
Remaining queries where we could improve are those with a small LIMIT (and many files per folder). I have left a TODO for what could be the strategy to improve there.