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

Implementation discussion: update-table changes #78

Closed
ricardojmendez opened this issue Dec 14, 2015 · 11 comments
Closed

Implementation discussion: update-table changes #78

ricardojmendez opened this issue Dec 14, 2015 · 11 comments
Assignees

Comments

@ricardojmendez
Copy link
Collaborator

I'm looking into expanding update-table to add other functionality, like GlobalSecondaryIndexUpdates, since as discussed on #77, it only currently supports provisioned throughput for the table.

I see the following issues:

  • Currently it expects a throughput map as a parameter, when you don't necessarily want to change the throughput for the table.
  • The map is specifically for throughput, when we should be receiving a map of values to change, for example, a description of :gsindexes updates to do.

I'm inclined to change that third parameter to update-table to just be a map with the settings for the operations to apply. This would be consistent with how the table description is passed to create-table and would solve both problems above, but since that'd be a breaking change I'd rather bring it up first.

Thoughts?

@ptaoussanis
Copy link
Member

Hi Ricardo,

The breaking change you have in mind sounds good to me, thanks for checking.

@ricardojmendez
Copy link
Collaborator Author

It looks like update-table's implementation right now is very throughput-specific, including mapping the update call on a series of throughput increase steps and returning the last one executed.

Is there a reason for this? I'm going through the referenced page but don't see a reason why the update request would be executed in steps, only the explanation for the decrease limit in a 24 hour period.

@ptaoussanis
Copy link
Member

Was the case previously that large throughput increases needed manual steps. Not sure if that's still the case or not. If not, that code's vestigial and can be removed.

@ricardojmendez
Copy link
Collaborator Author

Perfect. I'll see about removing it and running some tests against an actual live instance, since I've read some remarks that the local instance disregards throughput settings (it referred to global secondary indexes, but I expect is a general characteristic).

@ptaoussanis
Copy link
Member

Ahh yes, limits like that would only apply to a live cloud instance. Not sure if this particular limit still exists though.

@ricardojmendez
Copy link
Collaborator Author

No problem, I'll test. If the limits no longer exist then things would be much simpler, since I can just unify all the table updates - otherwise it'd be best to keep separate update-table and update-table-throughput functions.

@ricardojmendez
Copy link
Collaborator Author

Testing against a live cloud instance, I can get the update to throughput to apply in a single step. Will prune that old code.

@ptaoussanis
Copy link
Member

Just confirming: that's for a large increase, yes? I.e. more than 2x or 4x the previous value?

@ricardojmendez
Copy link
Collaborator Author

Correct, tried it going from {:read 1 :write 1} to {:read 16 :write 16} without stepping. No error and update returned the expected values

; First increase
{:lsindexes nil,
 :gsindexes nil,
 :name :temp_table_1450109809826,
 :throughput
 {:read 16,
  :write 16,
  :last-decrease nil,
  :last-increase #inst "2015-12-14T16:16:58.340-00:00",
  :num-decreases-today 0},
 :prim-keys {:artist {:key-type :hash, :data-type :s}},
 :size 0,
 :status :active,
 :item-count 0,
 :creation-date #inst "2015-12-14T16:16:49.943-00:00",
 :indexes nil}
; Second increase
{:lsindexes nil,
 :gsindexes nil,
 :name :temp_table_1450109809826,
 :throughput
 {:read 256,
  :write 256,
  :last-decrease nil,
  :last-increase #inst "2015-12-14T16:17:03.542-00:00",
  :num-decreases-today 0},
 :prim-keys {:artist {:key-type :hash, :data-type :s}},
 :size 0,
 :status :active,
 :item-count 0,
 :creation-date #inst "2015-12-14T16:16:49.943-00:00",
 :indexes nil}

@ptaoussanis
Copy link
Member

Great, happy to see that they've removed that limit- thanks for picking up on this.

ricardojmendez added a commit that referenced this issue Dec 15, 2015
This commit only changes throughput so that it's passed as an option in
a map, as discussed on #78. It's still required, which it shouldn't be
since it's not the only option to change.
ricardojmendez added a commit that referenced this issue Dec 15, 2015
@ricardojmendez
Copy link
Collaborator Author

Closing along with #80

ricardojmendez added a commit that referenced this issue Dec 17, 2015
This commit only changes throughput so that it's passed as an option in
a map, as discussed on #78. It's still required, which it shouldn't be
since it's not the only option to change.
ricardojmendez added a commit that referenced this issue Dec 17, 2015
ricardojmendez added a commit that referenced this issue Dec 19, 2015
* Renamed environment sample, more useful as a script
* Global secondary index tests for create-table
* Local secondary index tests

* BREAKING CHANGE: update-table now receives options in a map [1]
This commit only changes throughput so that it's passed as an option in
a map, as discussed on #78. It's still required, which it shouldn't be
since it's not the only option to change.

* We no longer need to do stepping when updating throughput
See discussion on #78

* Elaborating on table-status-watch comment
* Tests for decreasing throughput
* Disregarding throughput updates that are the same as the current values
Found that updateTable hangs if we pass the same throughput values as
the table currently has (see #79).  Refactored throughput parameter
validation to its own function, since we'll need to validate the
parameters for the other operations as well.

* Expanded tests.
* Updating encore and AWS SDK version, all tests passing
Want to make sure we're running against the latest version, to avoid
behavior like #79 being a known (and possibly solved) issue.

* Upgraded expectations, re-wrote tests that used expect-let
* Sending empty map to update-table should have no effect
See 8574687, this test should have gone in with that commit.

* Global secondary index creation on update-table, see #80
* update-table can now change throughput on :gsindexes (#80)
Also, new function: index-status-watch

* update-table can now delete :gsindexes (#80)
* Support for specifying index on scan (#82)
* Supporting projection and expr-attr-names on scan (#82)
* Added support for conditional deletes (#84)
* Added missing tests for expressions on update-item-request
* BREAKING CHANGE: update-map is now another value on update-item's params
Updated tests, fixed deprecation warning. See #83.

* Expanded docstring for update-table (#80)
ricardojmendez added a commit that referenced this issue Dec 22, 2015
* Renamed environment sample, more useful as a script
* Global secondary index tests for create-table
* Local secondary index tests

* BREAKING CHANGE: update-table now receives options in a map [1]
This commit only changes throughput so that it's passed as an option in
a map, as discussed on #78. It's still required, which it shouldn't be
since it's not the only option to change.

* We no longer need to do stepping when updating throughput
See discussion on #78

* Elaborating on table-status-watch comment
* Tests for decreasing throughput
* Disregarding throughput updates that are the same as the current values
Found that updateTable hangs if we pass the same throughput values as
the table currently has (see #79).  Refactored throughput parameter
validation to its own function, since we'll need to validate the
parameters for the other operations as well.

* Expanded tests.
* Updating encore and AWS SDK version, all tests passing
Want to make sure we're running against the latest version, to avoid
behavior like #79 being a known (and possibly solved) issue.

* Upgraded expectations, re-wrote tests that used expect-let
* Sending empty map to update-table should have no effect
See 8574687, this test should have gone in with that commit.

* Global secondary index creation on update-table, see #80
* update-table can now change throughput on :gsindexes (#80)
Also, new function: index-status-watch

* update-table can now delete :gsindexes (#80)
* Support for specifying index on scan (#82)
* Supporting projection and expr-attr-names on scan (#82)
* Added support for conditional deletes (#84)
* Added missing tests for expressions on update-item-request
* BREAKING CHANGE: update-map is now another value on update-item's params
Updated tests, fixed deprecation warning. See #83.

* Expanded docstring for update-table (#80)
ricardojmendez added a commit that referenced this issue Dec 22, 2015
* Renamed environment sample, more useful as a script
* Global secondary index tests for create-table
* Local secondary index tests

* BREAKING CHANGE: update-table now receives options in a map [1]
This commit only changes throughput so that it's passed as an option in
a map, as discussed on #78. It's still required, which it shouldn't be
since it's not the only option to change.

* We no longer need to do stepping when updating throughput
See discussion on #78

* Elaborating on table-status-watch comment
* Tests for decreasing throughput
* Disregarding throughput updates that are the same as the current values
Found that updateTable hangs if we pass the same throughput values as
the table currently has (see #79).  Refactored throughput parameter
validation to its own function, since we'll need to validate the
parameters for the other operations as well.

* Expanded tests.
* Updating encore and AWS SDK version, all tests passing
Want to make sure we're running against the latest version, to avoid
behavior like #79 being a known (and possibly solved) issue.

* Upgraded expectations, re-wrote tests that used expect-let
* Sending empty map to update-table should have no effect
See 8574687, this test should have gone in with that commit.

* Global secondary index creation on update-table, see #80
* update-table can now change throughput on :gsindexes (#80)
Also, new function: index-status-watch

* update-table can now delete :gsindexes (#80)
* Support for specifying index on scan (#82)
* Supporting projection and expr-attr-names on scan (#82)
* Added support for conditional deletes (#84)
* Added missing tests for expressions on update-item-request
* BREAKING CHANGE: update-map is now another value on update-item's params
Updated tests, fixed deprecation warning. See #83.

* Expanded docstring for update-table (#80)
ptaoussanis pushed a commit that referenced this issue Jan 23, 2016
* Renamed environment sample, more useful as a script
* Global secondary index tests for create-table
* Local secondary index tests

* BREAKING CHANGE: update-table now receives options in a map [1]
This commit only changes throughput so that it's passed as an option in
a map, as discussed on #78. It's still required, which it shouldn't be
since it's not the only option to change.

* We no longer need to do stepping when updating throughput
See discussion on #78

* Elaborating on table-status-watch comment
* Tests for decreasing throughput
* Disregarding throughput updates that are the same as the current values
Found that updateTable hangs if we pass the same throughput values as
the table currently has (see #79).  Refactored throughput parameter
validation to its own function, since we'll need to validate the
parameters for the other operations as well.

* Expanded tests.
* Updating encore and AWS SDK version, all tests passing
Want to make sure we're running against the latest version, to avoid
behavior like #79 being a known (and possibly solved) issue.

* Upgraded expectations, re-wrote tests that used expect-let
* Sending empty map to update-table should have no effect
See 8574687, this test should have gone in with that commit.

* Global secondary index creation on update-table, see #80
* update-table can now change throughput on :gsindexes (#80)
Also, new function: index-status-watch

* update-table can now delete :gsindexes (#80)
* Support for specifying index on scan (#82)
* Supporting projection and expr-attr-names on scan (#82)
* Added support for conditional deletes (#84)
* Added missing tests for expressions on update-item-request
* BREAKING CHANGE: update-map is now another value on update-item's params
Updated tests, fixed deprecation warning. See #83.

* Expanded docstring for update-table (#80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants