Bug 201561 – results.webkit.org: Configurations should be branch specific

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

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).

[Jonathan Bedard]

Created attachment 378240 [details]
Patch

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 378409 [details]
Patch

[Jonathan Bedard]

Created attachment 378414 [details]
Patch

[Jonathan Bedard]

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.

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 378871 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378871 [details]
Patch

Clearing flags on attachment: 378871

Committed r249924: <https://trac.webkit.org/changeset/249924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55417843>