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

CosmosStore - Extend aux collection initialization #328

Merged
merged 3 commits into from
May 27, 2022

Conversation

brihadish
Copy link
Contributor

Modifies initialization of aux collection to take an explicit input representing the cosmos provisioning mode. This change will be used in 'Propulsion.Tool' for supporting autoscale option and 'serverless' cosmos database.

@CLAassistant
Copy link

CLAassistant commented May 26, 2022

CLA assistant check
All committers have signed the CLA.

@bartelink
Copy link
Collaborator

bartelink commented May 27, 2022

Firstly, thanks for taking the time to do a PR. Now is definitely a reasonable time to introduce a breaking change like this given that we are in a -beta phase (but do let me know if you need a cherrypick of this to go into a 3.0.8 release as an extra function alongside the existing one for backcompat...).

In general I don't have an objection to the removal of the hardcoding of a default; the default stance on matters like this in Equinox is to push all these concerns outward as you are doing, and I agree there is no value in Equinox being the lib to take an opinionated stance on something as subtle as this which is ultimately a nuanced thing. (If you look at the code history, you'll see that this probably should have been realized and resolved when the autoscale support was added).

I have a question though... Can you share the configuration you've arrived at and the reasoning behind that? It will help others understand, and perhaps it might reveal a better solution.

I'm slightly concerned that e.g. autoscale being too aggressive would cause lease drop and reassignment storms.

The other thing is that appropriate wiring should probably follow to surface the facility in Propulsion.Tool

My aim would be that someone randomly running that tool would probably end up with the prev manual provisioning of 400, and not blindly go in with a more exotic configuration without understanding the implications. The dream would be to have a section in Propulsion's DOCUMENTATION.md with an overview and/or links to guidance on the topic.

An example for me of the subtleties involved: Azure/azure-cosmos-dotnet-v3#3215 (comment)

The relevant bits in Equinox.Tool to borrow would be

  • and [<NoComparison; NoEquality>]InitArguments =
    | [<AltCommandLine "-ru">] Rus of int
    | [<AltCommandLine "-A">] Autoscale
    | [<AltCommandLine "-m">] Mode of CosmosModeType
    | [<AltCommandLine "-P">] SkipStoredProc
    | [<CliPrefix(CliPrefix.None)>] Cosmos of ParseResults<Storage.Cosmos.Arguments>
    interface IArgParserTemplate with
    member a.Usage = a |> function
    | Rus _ -> "Specify RU/s level to provision for the Container (Default: 400 RU/s or 4000 RU/s if autoscaling)."
    | Autoscale -> "Autoscale provisioned throughput. Use --rus to specify the maximum RU/s."
    | Mode _ -> "Configure RU mode to use Container-level RU, Database-level RU, or Serverless allocations (Default: Use Container-level allocation)."
    | SkipStoredProc -> "Inhibit creation of stored procedure in specified Container."
    | Cosmos _ -> "Cosmos Connection parameters."
    and CosmosInitInfo(args : ParseResults<InitArguments>) =
    member _.ProvisioningMode =
    let throughput () =
    if args.Contains Autoscale
    then CosmosInit.Throughput.Autoscale (args.GetResult(Rus, 4000))
    else CosmosInit.Throughput.Manual (args.GetResult(Rus, 400))
    match args.GetResult(Mode, CosmosModeType.Container) with
    | CosmosModeType.Container -> CosmosInit.Provisioning.Container (throughput ())
    | CosmosModeType.Db -> CosmosInit.Provisioning.Database (throughput ())
    | CosmosModeType.Serverless ->
    if args.Contains Rus || args.Contains Autoscale then raise (Storage.MissingArg "Cannot specify RU/s or Autoscale in Serverless mode")
    CosmosInit.Provisioning.Serverless
  • let containerAndOrDb log (iargs: ParseResults<InitArguments>) = async {
    match iargs.TryGetSubCommand() with
    | Some (InitArguments.Cosmos sargs) ->
    let skipStoredProc = iargs.Contains InitArguments.SkipStoredProc
    let client, dName, cName = connect log sargs
    let mode = (CosmosInitInfo iargs).ProvisioningMode
    match mode with
    | CosmosInit.Provisioning.Container ru ->
    let modeStr = "Container"
    log.Information("Provisioning `Equinox.CosmosStore` Store at {mode:l} level for {rus:n0} RU/s", modeStr, ru)
    | CosmosInit.Provisioning.Database ru ->
    let modeStr = "Database"
    log.Information("Provisioning `Equinox.CosmosStore` Store at {mode:l} level for {rus:n0} RU/s", modeStr, ru)
    | CosmosInit.Provisioning.Serverless ->
    let modeStr = "Serverless"
    log.Information("Provisioning `Equinox.CosmosStore` Store in {mode:l} mode with automatic RU/s as configured in account", modeStr)
    return! CosmosInit.init log client (dName, cName) mode skipStoredProc
    | _ -> failwith "please specify a `cosmos` endpoint" }

You should be able to do a draft PR by changing the PackageReference for Equinox.CosmosStore over there to a local ProjectReference


Having said that, if you have a definite need for this and intend to simply call this API yourself directly, I definitely don't have an objection to the removal of the hardcoding of a default.

Assuming you wish to go ahead with the PR, can you also add a changelog.md entry please?

@brihadish
Copy link
Contributor Author

Thanks for the review :).

The reasoning behind this PR is that we have been getting compliance requirements from our DevOps team to move to 'serverless' dbs for non-prod environments and for universally applying 'autoscale' setting in collections pertaining to 'provisioned throughput' dbs. The aux collection was preventing us from undertaking an endeavor meet the aforementioned compliance requirements.

I do agree that a setting 400 RUs for the aux collection should be more than sufficient and I have already done the necessary changes to Propulsion.Tool which exactly matches your proposal (Default behavior of manual 400 RUs for backward compatibility. 'Autoscale' option will use a default max RU of 4000 RUs so that the fixed cost is same as manual mode.) I was unsure how to push the Propulsion tool changes without an updated nuget for 'Equinox.CosmosStore'.

Please correct if I'm wrong. I think using a local 'ProjectReference' will include bringing multiple projects into the propulsion PR. Is that fine? Nevertheless, I will create a draft PR of propulsion tool without the reference fix. I will also make necessary changes to the propulsion tool documentation.

I will certainly make an entry in changelog.md and update the current PR.

@bartelink
Copy link
Collaborator

I was unsure how to push the Propulsion tool changes without an updated nuget for 'Equinox.CosmosStore'.
You are correct that it's not possible to have it build on CI without putting lots of source in the PR branch etc, but you can show the code as intended and get the bulk of the view out of the way.

So what I meant is for you to do it locally, push the Propulsion bits and leave the PR in Draft state on github.

Please correct if I'm wrong. I think using a local 'ProjectReference' will include bringing multiple projects into the propulsion PR. Is that fine? Nevertheless, I will create a draft PR of propulsion tool without the reference fix. I will also make necessary changes to the propulsion tool documentation.

For it to work happily locally you would need to add ../equinox/src/Equinox.CosmosStore to the sln (I think it will be OK without Equinox.Core and Equinox)

Any docs piece would be wonderful.


Once you have the changelog bit pushed, I can stage a release of Equinox which you can target once it gets to nuget.

Is having it in Equinox.CosmosStore 4.0.0-beta.X and your changes being in Propulsion.Tool-2.13.0-beta.Y OK ?

(I dont any significant changes in Propulsion or Equinox but I'll be on vacation for a period and would prefer to not make badge them with full released numbers until I'm ready to do the associated changes for dotnet-templates)

@brihadish
Copy link
Contributor Author

brihadish commented May 27, 2022

Propulsion tool PR is jet/propulsion#142. Using beta package for the propulsion tool will be fine. I have pushed the edit to changelog.

@bartelink bartelink merged commit 0115950 into jet:master May 27, 2022
@bartelink
Copy link
Collaborator

Thanks for your contribution! Will ping you here when the nuget upload has completed (there's a manual step in the chain; no eta as yet)

@deviousasti
Copy link
Member

Packages released as beta6.

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.

None yet

4 participants