Conversation
googlebot
added
the
cla: yes
|
@rameshdharan thinking this sample is better here |
|
|
||
| def test_downscale(): | ||
| downscale_strategy = DOWNSCALE_STRATEGIES['incremental'] | ||
| assert downscale_strategy(5) == 3 |
There was a problem hiding this comment.
How do you run these tests without using the unittest module? Are they imported somewhere into a main file which runs all the tests?
There was a problem hiding this comment.
Our main test runner is pytest which scans for functions that start with test_ and handles the Python asserts in a way similar to how unittest would the assert functions, so it's pretty neat.
If you do:
pip install pytest
pytest
it will run all the tests.
In this specific repo, since there's dozens of samples ,we use nox which helps instrument different sessions etc. But really in here it will just call pytest.
bigtable/autoscaler/autoscaler.py
Outdated
|
|
||
| import strategies | ||
|
|
||
| CPU_METRIC = 'bigtable.googleapis.com/cluster/cpu_load' |
There was a problem hiding this comment.
while constants are good practice in production code, in sample code they add a layer of indirection that can be annoying when including a snippet in cloud site. Strongly prefer this to go near where it's used.
bigtable/autoscaler/autoscaler.py
Outdated
| """ | ||
| client = monitoring.Client() | ||
| query = client.query(CPU_METRIC, minutes=5) | ||
| return list(query)[0].points[0].value |
There was a problem hiding this comment.
For sample code, prefer pulling out discrete values and giving them names:
results = client.query(CPU_METRIC, minutes=5)
results = list(query)
first_result = results[0]
cpu_load = first_result.points[0].valueAlso, shouldn't that query have a limit of 1?
There was a problem hiding this comment.
Done. API doesn't support limits on queries.
bigtable/autoscaler/autoscaler.py
Outdated
|
|
||
| Args: | ||
| bigtable_instance (str): Cloud Bigtable instance id to scale | ||
| up (bool): If true, scale up, otherwise scale down |
There was a problem hiding this comment.
I'd recommend scale_up as a more more descriptive name. Then, you can probably drop the args section and just describe the behavior in the description.
There was a problem hiding this comment.
scale_up seems misleading in the case of downscale.
There was a problem hiding this comment.
And up is less misleading?
There was a problem hiding this comment.
Sorry, misunderstood, thought you meant the function name. Done.
| default=60 * 10) | ||
| args = parser.parse_args() | ||
|
|
||
| while True: |
There was a problem hiding this comment.
I had it in main, but I moved it out here to make it easier to test main. Otherwise testing while True loops is annoying. Would probably have to make a bunch of changes just to test the code, this seemed like the simpler option.
There was a problem hiding this comment.
The tests shouldn't influence flow control. We can figure out how to break out of the loop. Likely by using mock to insert a keyboard interrupt.
| @@ -0,0 +1,50 @@ | |||
| # Copyright 2017 Google Inc. | |||
There was a problem hiding this comment.
Any reason to have this as a separate file? For the sake of samples, it seems it would be most useful to locate this near the code that uses these strategies.
There was a problem hiding this comment.
The idea is we are going to add a few alternative strategies such as exponential, and have them all organized in here.
There was a problem hiding this comment.
I see. If that's valuable and matches with how this will be presented in the docs, then that's fine.
There was a problem hiding this comment.
Ya doc is still a WIP so once we have a draft of that I'll take another look at this and if it's pointlessly complicating things I'll move it back into autoscaler.py.
|
@waprin why was this merged? There was at least one outstanding comment that I wanted to resolve before we merged. I'm usually fine with compromises in the near term as long as we create bugs to follow up, but dismissing my review and merging does not seem like our usual mode of operation. What's the rush to merge this? |
* chore(deps): update all dependencies * Update docs.yml * Update lint.yml * Update requirements.txt --------- Co-authored-by: Lingqing Gan <lingqing.gan@gmail.com>
4 participants
No description provided.