The fact that configurations are global, rather than being applied per branch, has lead to a number of idiosyncrasies, most notably, the user being provided with more configurations to filter by than are actually available. To fix this, we need to bucket configurations by branch. Note that this will require a small database migration (only a handful of very small tables).
Created attachment 378240 [details] Patch
I'd like a sanity check on this before I push it to the dev instance, since even on the dev instance, it makes some database schema changes.
Created attachment 378409 [details] Patch
Created attachment 378414 [details] Patch
I talked to Kocsen about this earlier in the week. We determined that using branch as a partitioning key made our life more difficult, so instead, I switched it to just a primary key. We still need to create new tables, but the code for extracting the configurations into the Redis cache is much smaller.
Comment on attachment 378414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378414&action=review > Tools/resultsdbpy/resultsdbpy/controller/upload_controller.py:170 > + branch=branch[0] if branch else None, Is there some mechanism to ensure branch is aways a list of string rather than just a string? > Tools/resultsdbpy/resultsdbpy/model/configuration_context.py:215 > + def search_for_recent_configuration(self, configuration=Configuration(), branch=None): Just want to point out that the default argument `configuration` will only be initialized once at the function def time. All invocations to this function will ref to a single configuration object if default value is used. > Tools/resultsdbpy/resultsdbpy/model/configuration_context_unittest.py:79 > + self.database.register_configuration(configuration, None, old) Personally, I would prefer to use `timestamp=old` over passing a None argument. > Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:120 > +let configurationsDefinedCallbacks = []; Where is this variable updated? > Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:134 > + configurations = []; > + json.forEach(pair => { > + const config = new Configuration(pair[0]); > + configurations.push(config); > + }); How about `configurations = json.map(pair => new Configuration(pair[0])`? > Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:135 > + configurationsDefinedCallbacks.forEach(callback => {callback();}); I think `configurationsDefinedCallbacks.forEach(callback => callback());` would work.
r=me with comment.
Comment on attachment 378414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378414&action=review >> Tools/resultsdbpy/resultsdbpy/controller/upload_controller.py:170 >> + branch=branch[0] if branch else None, > > Is there some mechanism to ensure branch is aways a list of string rather than just a string? Yes, @query_as_kwargs() turns the request's query into a kwarg dictionary, and always passes arguments as a list. >> Tools/resultsdbpy/resultsdbpy/model/configuration_context.py:215 >> + def search_for_recent_configuration(self, configuration=Configuration(), branch=None): > > Just want to point out that the default argument `configuration` will only be initialized once at the function def time. All invocations to this function will ref to a single configuration object if default value is used. That's totally fine, we convert the configuration to a string in self.redis.scan_iter(...) and don't make any edits to it. Sometimes I wish Python had a 'const' keyword.... >> Tools/resultsdbpy/resultsdbpy/view/static/js/drawer.js:120 >> +let configurationsDefinedCallbacks = []; > > Where is this variable updated? It's updated in ConfigurationSelectors(...), it was originally a local variable within that function, we're just making it local to the whole file.
Created attachment 378871 [details] Patch
Comment on attachment 378871 [details] Patch Clearing flags on attachment: 378871 Committed r249924: <https://trac.webkit.org/changeset/249924>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55417843>