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 settings for autoscaling to Bigtable module. #1006

Merged
merged 7 commits into from
Nov 24, 2022

Conversation

iht
Copy link
Member

@iht iht commented Nov 23, 2022

Bigtable clusters can now autoscale, adding the corresponding settings to the bigtable-instance.

The default num_workers must be set to null, because setting num_workers cannot be done at the same time as specifying autoscaling_config.

@iht iht enabled auto-merge November 23, 2022 13:31
@lcaggio
Copy link
Collaborator

lcaggio commented Nov 23, 2022

At the moment autoscaling_config and num_nodes have a null default.

If not wrong: if the user relies on module defaults, an instance with unset num_nodes will be created (suggested for DEV). I would improve the num_nodes description suggesting to set num_nodes > 0 or set the auto_scaling variable for a prod env.

@iht iht disabled auto-merge November 23, 2022 14:11
@iht
Copy link
Member Author

iht commented Nov 23, 2022

At the moment autoscaling_config and num_nodes have a null default.

If not wrong: if the user relies on module defaults, an instance with unset num_nodes will be created (suggested for DEV). I would improve the num_nodes description suggesting to set num_nodes > 0 or set the auto_scaling variable for a prod env.

If not num_nodes nor autoscaling_config are set, the creation of the instance fails because the number of nodes is understood to be zero (default value is null). So the user is safe, no instance would be created in that situation.

I have added some more remarks about this situation in the documentation.

See commits e05dec9 and 59e1d13.

@iht iht enabled auto-merge November 23, 2022 14:20
@lcaggio
Copy link
Collaborator

lcaggio commented Nov 23, 2022

I see. In this case, the user has to set an optional variable otherwise the module won't work.

What about having a logic to not set num_nodes on the resource if the value is null? This should create a resource suitable for DEV (see resource documentation)

@iht iht requested a review from lcaggio November 24, 2022 13:45
@lcaggio
Copy link
Collaborator

lcaggio commented Nov 24, 2022

num_nodes = null requires a instance_type configuration. instance_type is deprecated.

I suggest do one of the following:

  • set a default to 1 for num_nodes and ignore if autoscaling_config is configured (mentioning the behaviour in the variable description).
  • create a single variable with 2 attribute: num_nodes and autoscaling_config and use condition to verify that only one of the 2 is configured.

In both ways the user can have a simple deployment just settings mandatory attributes.

@iht
Copy link
Member Author

iht commented Nov 24, 2022

num_nodes = null requires a instance_type configuration. instance_type is deprecated.

I suggest do one of the following:

  • set a default to 1 for num_nodes and ignore if autoscaling_config is configured (mentioning the behaviour in the variable description).
  • create a single variable with 2 attribute: num_nodes and autoscaling_config and use condition to verify that only one of the 2 is configured.

In both ways the user can have a simple deployment just settings mandatory attributes.

I have implemented the first option in commit 5d38e31.

Thanks for the suggestions!

Copy link
Collaborator

@lcaggio lcaggio left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@iht iht merged commit 5a6ed05 into GoogleCloudPlatform:master Nov 24, 2022
@juliocc
Copy link
Collaborator

juliocc commented Nov 24, 2022

nice branch name

@iht iht deleted the bigtable_grooming branch November 27, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants