Skip to content

Add Bigtable Autoscaler sample #943

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

Merged
merged 12 commits into from
May 16, 2017
Merged

Add Bigtable Autoscaler sample #943

merged 12 commits into from
May 16, 2017

Conversation

waprin
Copy link
Contributor

@waprin waprin commented May 13, 2017

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 13, 2017
@waprin waprin requested a review from ace-n May 13, 2017 00:53
@waprin waprin removed the request for review from ace-n May 13, 2017 00:54
@waprin
Copy link
Contributor Author

waprin commented May 13, 2017

@rameshdharan thinking this sample is better here

@waprin waprin requested review from tswast and theacodes May 13, 2017 00:54

def test_downscale():
downscale_strategy = DOWNSCALE_STRATEGIES['incremental']
assert downscale_strategy(5) == 3
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you run these tests without using the unittest module? Are they imported somewhere into a main file which runs all the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


import strategies

CPU_METRIC = 'bigtable.googleapis.com/cluster/cpu_load'
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"""
client = monitoring.Client()
query = client.query(CPU_METRIC, minutes=5)
return list(query)[0].points[0].value
Copy link
Contributor

Choose a reason for hiding this comment

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

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].value

Also, shouldn't that query have a limit of 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. API doesn't support limits on queries.


Args:
bigtable_instance (str): Cloud Bigtable instance id to scale
up (bool): If true, scale up, otherwise scale down
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scale_up seems misleading in the case of downscale.

Copy link
Contributor

Choose a reason for hiding this comment

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

And up is less misleading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, misunderstood, thought you meant the function name. Done.

default=60 * 10)
args = parser.parse_args()

while True:
Copy link
Contributor

Choose a reason for hiding this comment

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

move the while to main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is we are going to add a few alternative strategies such as exponential, and have them all organized in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. If that's valuable and matches with how this will be presented in the docs, then that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 waprin merged commit 47baea9 into master May 16, 2017
@theacodes
Copy link
Contributor

@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?

@theacodes theacodes deleted the bigtable_autoscaler branch June 28, 2017 16:06
Linchin added a commit that referenced this pull request Aug 18, 2025
* chore(deps): update all dependencies

* Update docs.yml

* Update lint.yml

* Update requirements.txt

---------

Co-authored-by: Lingqing Gan <lingqing.gan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants