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

Clarify ctor/Creation/Verification lifecycle #43

Merged
merged 22 commits into from
Apr 12, 2022

Conversation

bartelink
Copy link
Member

@bartelink bartelink commented Mar 28, 2022

Replaces TableContext.Create and associated APIs with a clearer set of APIs that facilitate better separation of what I see as multiple phases of processing, depending on how one deploys one's app (informed by how Equinox's tooling and conventions separates this in larger applications).

The split is:

  1. Being able to create a TableContext with the assumption that the underlying table and requisite indexes are in place (without any external calls), via the constructor
  2. VerifyTableAsync to check that the tableName specified by the constructor is in place and has the correct indices in place (Scripting.Initialize combines this step with the constructor for scripting scenarios). Does not create a table; throws if there is a problem. Should ideally only be called once per process startup.
  3. VerifyOrCreateTableAsync - as per VerifyTableAsync but will create the table if it happens not to be present. For small single EXE apps, or test scenarios, calling this once on startup may make sense. Can also be used from separated configuration logic; useful for any application scenario where the creation of tables is something that is managed via a configuration phase of a deployment strategy (DDL vs DML separation in SQL DB tech parlance)
  4. UpdateTableIfRequiredAsync - can apply adjustments to Provisioning or Streaming configuration via the UpdateTable API if the table's configuration is not as specified. Used to implement Equinox's CLI tooling wrt establishing and/or changing throughput an/or streaming configuration.

resolves #35

@bartelink
Copy link
Member Author

see also related pre-discussion thread with @samritchie

@bartelink bartelink force-pushed the init-reorg branch 2 times, most recently from e05cdc4 to a6515b0 Compare March 28, 2022 12:16
@bartelink bartelink changed the title Clarify ctor and Creation/Verification APIs Clarify ctor/Creation/Verification lifecycle Mar 28, 2022
@bartelink bartelink mentioned this pull request Mar 28, 2022
@bartelink
Copy link
Member Author

@samritchie I ended up havig to put an ugly guard into

| InitializationMode.CreateOrUpdateThroughput t ->
let provisioned = td.Table.ProvisionedThroughput
if t.ReadCapacityUnits <> provisioned.ReadCapacityUnits
|| t.WriteCapacityUnits <> provisioned.WriteCapacityUnits then
do! __.UpdateProvisionedThroughputAsync(t)

If you search for CreateOrUpdateThroughput in that file, you can see its relatively low impact, but I definitely don't like the the coupling that guard adds.
Pending a commit now to remove it (checking if it makes sense to expose the Describe API, but I guess that's pretty debatable too...)

@bartelink bartelink force-pushed the init-reorg branch 2 times, most recently from 2b403f1 to 60a166b Compare March 28, 2022 16:28
@bartelink
Copy link
Member Author

@samritchie Refactored to put my expose my semantics in TableContext.ProvisionTableAsync without exposing that DU I had.

The choices now from my perspective are:

  1. give FSharp.AWS.DynamoDb the burden of maintaining ProvisionTableAsync's checking of the TableDescription (arguably that makes sense as it's kinda the inverse of what it owns via InternalCreateTableRequest)
  2. make InternalCreateOrValidateTableAsync public
  3. provide an even more general solution by exposing InternalProvision

I'm happy to do any more cleanup you think makes sense, as long as I wind up with some code (that does not involve that cut and pasting the retry loop!) to to pop into my provisioning impl in https://github.com/jet/equinox/blob/acccec32fabb27ec03b731a9a1c340fb341e3ae7/src/Equinox.DynamoStore/DynamoStore.fs#L472-L473
(That will ultimately be exposed via something like dotnet tool install Equinox.Tool && eqx init dynamo -rru 10 -wru 100 -s https://localhost:8000 -t my-table-name)

@bartelink bartelink force-pushed the init-reorg branch 2 times, most recently from c93555e to 54cbdd6 Compare March 29, 2022 16:41
@bartelink
Copy link
Member Author

bartelink commented Mar 29, 2022

@samritchie I worked in support for OnDemand mode - the throughput argument for the new Initialize/Provision APIs is now a DU
Let me know if you want me to shift that (or any other aspect) into a separate PR
(The question of whether provisioning logic belongs in here remains - InternalCreateCreateTableRequest and ProvisionAsync are where the mapping is managed now)

@bartelink
Copy link
Member Author

@samritchie added support for configuring Streaming mode to manage an Equinox feature. High level:

  • eqx initAws -rru 10 -wru 20 dynamo creates or applies streaming mode to the table
  • eqx initAws -D dynamo changes the table to PAY_PER_REQUEST (and updates the streaming mode if it has drifted)
  • (re-issuing initial command switches it back)

In each of the above cases, you want to achieve a desired state, which means you need to

  • inspect the current state
  • only do an UpdateTable request if one or both of the settings has changed (it'll throw otherwise)
  • wait for the correct state to be established, i.e. that retry loop
  • report exceptions (if you exceed the maximum changes within a time window you want the operator to know via eqx returning a non-zero exit code)

The exact way in which the Throughput and Streaming DUs are exposed should can be changed and/or reconsidered, but I think the high level flow of having CreateTableAsync / VerifyTableAsync + UpdateTableAsync + ProvisionTableAsync seems to make sense regardless of whether both Throughput and Streaming belong in the box as such.

@bartelink
Copy link
Member Author

@samritchie I've topped and tailed this and can't think of anything else significant I need in the context of jet/equinox#321 so am happy to clean up as you see fit now

@bartelink bartelink force-pushed the init-reorg branch 3 times, most recently from a8bb3fb to d72d9d2 Compare April 6, 2022 09:32
@samritchie
Copy link
Collaborator

@bartelink I'll make some time tomorrow to pull this branch and have a good run through. One of the main things I'm trying to work out is GSI provisioning - currently GSIs are set to the table provisioning levels on Create, but not modified on Update (I don’t think you’ve changed that). GSIs will often have the same write capacity and a different read capacity to the table. This is going to be a really low usage feature though, maybe it’s enough to offer the customize callback and people can modify as they see fit.

@bartelink
Copy link
Member Author

Thanks @samritchie I've just completed local testing now and it's happy (I had a flipped condition in the td.Table.TableStatus <> TableStatus.ACTIVE bit 🤦‍♂️)

I have no immediate plans to use GSIs so definitely can't help beyond agreeing that yes, having it provisioned as per the actual table smells.
One could add a case to

type Throughput =
    | Provisioned of ProvisionedThroughput
    | OnDemand

If you actually had a plan ;)

@samritchie
Copy link
Collaborator

@bartelink I had a bit of a go at the new initialisation APIs including some testing on a real account. Some of the notable behaviour I found with cloud Dynamo was:

  • Attempting to update provisioning on an existing table with the same values as currently defined on that table will throw an Amazon.DynamoDBv2.AmazonDynamoDBException.
  • Attempting to update provisioning on an existing table while a provisioning update is already underway will throw a Amazon.DynamoDBv2.Model.ResourceInUseException (note changing capacity on a large table can be very time-consuming, applying OnDemand also seems to be a lengthy op).
  • Decreasing provisioning levels is rate-limited (once per hour after the first 4 in a day) and will throw a Amazon.DynamoDBv2.Model.LimitExceededException if exceeded.
  • There are a few other rules that will trigger Amazon.DynamoDBv2.Model.LimitExceededException - exceeding max capacity, switching to OnDemand appears to be rate-limited to once per day etc.

The upshot is I'm not sure how useful/usable it is to update provisioning on an existing table as part of table initialisation - ie ProvisionTableAsync. It’s highly likely that this method will throw (especially in a concurrent environment), but it’s also likely the provisioning exception is not critical and could be caught and logged/ignored - as opposed to a create/verify exception which will normally be fatal. IMO it’s going to be better to leave this decision to the consumer via an InitializeTableAsync/try UpdateTableAsync pair. I think it’s also worth adding some doc comments to UpdateTableAsync re the exceptions.

Initialising a context via constructor then VerifyAsync/InitialiseAsync definitely feels more painful to use from code (for people currently using those options) - I guess the gamble is the majority of people also found it as useless as I have in a real-world application. Hopefully the comments on the obsolete Create make the trade-offs clear.

I know I complained about ambiguous use of the term 'Create' but I'm wondering if CreateTableAsync may be clearer regarding its behaviour than InitializeTableAsync (particularly as it’s no longer a factory)? I suspect people would be fine with the semantics of it being 'create if needed'. Happy to hear your opinions there.

@bartelink
Copy link
Member Author

Right, have implemented as above - let me know your thoughts on how to proceed on the open conversations and I'll get to them asap...

@samritchie samritchie merged commit 8d4c042 into fsprojects:master Apr 12, 2022
@bartelink bartelink deleted the init-reorg branch April 12, 2022 06:37
@bartelink
Copy link
Member Author

bartelink commented Apr 12, 2022

Apologies for the confusion re those breaking changes (which were intentional on my part, even if in retrospect pretty ill-judged 😊) @samritchie

I've integrated with 0.10.1-beta in jet/equinox@dcbc036 and it all seems happy - thanks for getting it out there!

Arguably #35 should be re-opened? (I had intended to remove the resolves ref to it in the overview of this PR after our discussion there regarding the need to provide mapping for epoch times re TTL)

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.

support for TTL
2 participants