Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default flank settings to optimize performance #644

Merged
merged 2 commits into from
Feb 28, 2020

Conversation

pawelpasterz
Copy link
Contributor

@pawelpasterz pawelpasterz commented Feb 28, 2020

Fixes #455

  • Nexuslowres can make executions up to 4x faster due to smaller screen size.
  • Disable video (results bucket significant smaller, skips processing time of generating video)
  • Disable auto login (saves 30s - 1m)
  • Disable performance metrics. (saves processing time)

Chnages

  • Add FlankDefaults class to preserve defaults Flank settings
  • Update unit tests
  • Update documentation

Checklist

  • Documented
  • Unit tested

@codecov-io
Copy link

Codecov Report

Merging #644 into master will decrease coverage by 0.03%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #644      +/-   ##
============================================
- Coverage     77.32%   77.29%   -0.04%     
  Complexity      613      613              
============================================
  Files            82       83       +1     
  Lines          2329     2330       +1     
  Branches        311      311              
============================================
  Hits           1801     1801              
- Misses          334      335       +1     
  Partials        194      194

* Values should assure best (quickest) test performance.
* Each of value can be overwritten by config *.yml file
*/
object FlankDefaults {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if hiding default values behind constants was necessary. Any of those constant are used only once and they are not telling more than members which they are assigned. But we have one file more to check, if we want to know defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general case I agree. In this particular one I wanted to point explicitly that those are not default, gcloud values, and what those values mean (ex when I see DISABLE_VIDEO_RECORDING I would conclude that Flank's default setting is to have this disabled not being particularly interested if this is false, -1 or sth else). Probably enum based would be better but this will require bigger refactor that's why I decided to proceed with this solution.

I could also just change true -> false and leave additional comment why it is implemented that way.

TBH both solutions are fine for me. Let me know what you think, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was only suggestion so you can leave it as is. Your arguments are reasonable, it's ok for me.

@pawelpasterz pawelpasterz merged commit 25ac2b8 into master Feb 28, 2020
@pawelpasterz pawelpasterz deleted the optimize-default-settings branch February 28, 2020 20:54
@pawelpasterz pawelpasterz self-assigned this Mar 1, 2020
@bootstraponline bootstraponline modified the milestone: May 2020 Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fast/stable by default
4 participants