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 support for on-demand billing to table migrations. #94

Merged
merged 1 commit into from
May 13, 2021

Conversation

alboyadjian
Copy link
Contributor

Accepts :billing_mode option, with values of PAY_PER_REQUEST or PROVISIONED.

If :provisioned_throughput is set, :billing_mode must either not be set or set to PROVISIONED.

If :provisioned_throughput is not set, :billing_mode must be set and must be PAY_PER_REQUEST.

Setting :billing_mode to PAY_PER_REQUEST is invalid if :provisioned_throughput is specified.

Global secondary index througput config is not needed if on-demand billing is set for the table.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.456% when pulling 3f126ac on vhl:on-demand-billing-migration into dd89701 on aws:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.456% when pulling 3f126ac on vhl:on-demand-billing-migration into dd89701 on aws:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.456% when pulling 3f126ac on vhl:on-demand-billing-migration into dd89701 on aws:master.

Base automatically changed from master to main March 8, 2021 19:14
@brcarp
Copy link

brcarp commented May 12, 2021

I discovered this issue recently. This pull request has had its second birthday. Are there any plans for it?

@brcarp
Copy link

brcarp commented May 12, 2021

cc/ @alextwoods

@alextwoods
Copy link
Contributor

Thanks for the ping on this! Reviewing now....

Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Very minor comments/questions. Needs a changelog entry as well for release

test_gsi: {
read_capacity_units: 1,
write_capacity_units: 1
context 'when the table is not configured ot use on-demand billing' do
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo, ot -> to

)
end
gsis_with_throughput = _add_throughout_to_gsis(gsis, gsit)
create_opts[:global_secondary_indexes] = gsis_with_throughput
gsis_opts = if opts.key?(:billing_mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this check billing_mode equal to PAY_PER_REQUEST?

@alboyadjian alboyadjian force-pushed the on-demand-billing-migration branch 2 times, most recently from 6acfc31 to 67800dc Compare May 12, 2021 20:34
Accepts :billing_mode option, with values of PAY_PER_REQUEST or
PROVISIONED.

If :provisioned_throughput is set, :billing_mode must either not be set
or set to PROVISIONED.

If :provisioned_throughput is not set, :billing_mode must be set and
must be PAY_PER_REQUEST.

Setting :billing_mode to PAY_PER_REQUEST is invalid if
:provisioned_throughput is specified.

Global secondary index througput config is not needed if on-demand
billing is set for the table.
@alboyadjian alboyadjian force-pushed the on-demand-billing-migration branch from 67800dc to 82819d1 Compare May 12, 2021 20:36
Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

LGTM!

@alextwoods
Copy link
Contributor

Looks good - thanks for submitting and apologies it took so long for us to get merged. I'll get a release with a new version out today.

@alextwoods alextwoods merged commit 15463f3 into aws:main May 13, 2021
@brcarp
Copy link

brcarp commented May 19, 2021

@alextwoods Thanks for merging this in! What needs to happen for tagging a new version release? Is there anything we can do to support that?

@alextwoods
Copy link
Contributor

Forgot to post here, but this change has been released with aws-record 2.6.0 that went out last week (May 13th).

@brcarp
Copy link

brcarp commented May 19, 2021

Yeah, I didn't see the release tagged here in the repository, although I see that the version was updated in the code.

@brcarp
Copy link

brcarp commented May 19, 2021

@alextwoods
Copy link
Contributor

Oh - thanks for the callout - usually our release process updates those automatically, but I had some issues with the release scripts. I've manually created/tagged the 2.6.0 release.

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.

4 participants