Rename converted operations from agg to facet, Solr's term - #78
serhiy-bzhezytskyy wants to merge 1 commit into
Conversation
An OpenSearch workload names its aggregation operations after the aggregation: nyc_taxis has date_histogram_agg and distance_amount_agg, geonames has country_agg_uncached, country_agg_cached and numeric-term-cardinality-agg-high. Solr calls that a facet, and the workloads already shipped in solr-orbit-workloads use the facet names — so every regeneration produced five operations the checked-in workloads do not have, and re-running the converter over a workload directory would silently rename them back. An operation name is not only the "name" key. It is what a test procedure schedules, the prefix of its <name>_iterations, _target_throughput, _search_clients and _time_period parameters, and the key workload.py registers a standard value source under. Renaming only the definition leaves a workload that loads and then cannot find its value source, so the rewrite covers all of them, longest name first so country_agg is not rewritten inside country_agg_cached. The token is bounded on both sides: aggs, aggregation and the aggs-query-* operations keep their names. The map is built from operations/ and from the operations of workload.json only, never from a test procedure, so a procedure named after an aggregation is left alone. Regenerating nyc_taxis and geonames from the upstream workloads now produces every operation name, test procedure reference, parameter prefix and value source the shipped workloads use, with nothing left only in the checked-in copy, and the only lines that change in the generated output are the names themselves. The converter's docstring promised a result dict of output_dir, issues and skipped operations, which stops being true here, so it and the two converter doc pages are updated with it.
3befc08 to
a54387c
Compare
|
I'm a bit reluctant to so much code for a naming. I considered this while doing the port but left it. I view the converter tool as a bootstrap tool to get you started, and then it will be a starting point, but expect the developer to tune and tweak it until it is ok for Solr. So perhaps we can accept some minors like this? I don't doubt that the code works, have not reviewed in detail, just my first reaction given the patch size, 277 lines more code to maintain. |
|
On your question: yes, minors like this can be accepted by hand, and pausing this PR is fine. On the converter more generally, the reason I keep sending fixes to it is that it is the only place the findings from a port get applied automatically. Every difference I correct by hand in a workload is one the next person will hit and correct by hand again, and nothing in the repo tells them it was already found. So when something has to be tuned manually, folding that back into the converter is what makes the next workload start closer to working instead of starting from the same wrong output. That is also why I would rather not treat naming and broken output as the same kind of change. #74 and #77 are the second kind: there the generated workload does not run. A space in a range term gives HTTP 400, and I will lead with that kind from now on, so the patch size question does not come up on changes that are only cosmetic. |
2 participants
Description
convert-workloadcopies operation names through, so a converted workload keeps OpenSearch's word for an aggregation —date_histogram_agg,country_agg_cached. The workloads in solr-orbit-workloads use the facet names, so these are the only names a regeneration does not reproduce.An operation name is also what a test procedure schedules, the prefix of its
<name>_iterationsparameters, and the keyworkload.pyregisters a value source under, so the rename covers all of them — renaming only the definition gives a workload that loads and then aborts with a missing value source. The token is matched on its own, soaggsandaggregationare untouched. Each rename is recorded inCONVERTED.md.Issues Resolved
None.
Testing
Unit tests for the token boundaries, all four reference sites and an end-to-end conversion. Regenerating nyc_taxis and geonames now produces every operation name the shipped workloads use, and the only lines that change in the generated output are the names themselves.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.