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.

Sign up for GitHub

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

Jump to bottom

Updated Cosmos DB Partitioned following feedback #2684

Merged
merged 5 commits into from
Oct 10, 2019
Merged

Updated Cosmos DB Partitioned following feedback #2684

merged 5 commits into from
Oct 10, 2019

Conversation

garypretty
Copy link
Contributor

  • Amends container creation to disable indexing
  • Switch to using ReadItemAsync instead of Query
  • Switch PartitionKey to use Id instead of RealId

@garypretty
Updates following code review feedback.
d441b74 
Use Id for partition key. Use ReadItemAsync instead of Query.
@garypretty
Amended container creation to disable indexing.
bee562a 
Removed redundant Throughput option - we will create container with default settings and allow user to amend in Azure.
christopheranderson
christopheranderson approved these changes Oct 9, 2019
View reviewed changes
Copy link

@christopheranderson christopheranderson left a comment

Choose a reason for hiding this comment

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

Cosmos specific logic looks good. 👍

FYI - it looks like you also stopped passing a throughput, so you'll get whatever the SDK thinks the default ought to be. That's probably fine, but you didn't mention it in your change notes, so wanted to call it out as it is potentially a behavioral change.

@garypretty
Copy link
Contributor Author

Thanks @christopheranderson - I couldn't see a way of specifying the throughput when defining my container. I don't believe this is an issue anyway as the user can manage afterwards in Azure if needed and I will ensure it is added to the sample comments as well. Cheers again!

j82w
j82w reviewed Oct 9, 2019
View reviewed changes
libraries/Microsoft.Bot.Builder.Azure/CosmosDbPartitionedStorage.cs Outdated
_cosmosDbStorageOptions.ContainerThroughput)
.DefineContainer(_cosmosDbStorageOptions.ContainerId, DocumentStoreItem.PartitionKeyPath)
.WithIndexingPolicy().WithAutomaticIndexing(false).WithIndexingMode(IndexingMode.None).Attach()
.CreateIfNotExistsAsync()
Copy link

Choose a reason for hiding this comment

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

https://docs.microsoft.com/en-us/dotnet/api/microsoft.azure.cosmos.database.definecontainer?view=azure-dotnet#Microsoft_Azure_Cosmos_Database_DefineContainer_System_String_System_String_

Suggested change
.CreateIfNotExistsAsync()
.CreateIfNotExistsAsync(_cosmosDbStorageOptions.ContainerThroughput)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@j82w Ah! Thank you! I will update.

@mdrichardson
Copy link
Contributor

@gary Should the tests include not throwing when deleting non-existing item? I'm not sure if it throws a 404 on the dotnet side, but I know it does in Node.

@garypretty
Copy link
Contributor Author

@mdrichardson ill check. Good shout I think. I'll add a test. We have a read unknown so we should probably have a delete unknown test too.

@mdrichardson mdrichardson mentioned this pull request Oct 9, 2019
Merged
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 82595

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1073 unchanged lines in 91 files lost coverage.
  • Overall coverage decreased (-0.6%) to 76.801%
Files with Coverage Reduction New Missed Lines %
/libraries/Adapters/Microsoft.Bot.Builder.Adapters.Twilio/TwilioAdapter.cs 1 85.71%
/libraries/integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.Core/ApplicationBuilderExtensions.cs 1 71.43%
/libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/ConfigurationChannelProvider.cs 1 75.0%
/libraries/Microsoft.Bot.Builder.AI.LUIS/Generator/Age.cs 1 75.0%
/libraries/Microsoft.Bot.Builder.AI.LUIS/Generator/Money.cs 1 75.0%
/libraries/Microsoft.Bot.Builder.AI.LUIS/Generator/Temperature.cs 1 75.0%
/libraries/Microsoft.Bot.Builder.AI.LUIS/LuisApplication.cs 1 95.45%
/libraries/Microsoft.Bot.Builder/BotStateSet.cs 1 85.0%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/NumberPrompt.cs 1 99.04%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/PromptRecognizerResult.cs 1 85.71%
Totals Coverage Status
Change from base Build 82147: -0.6%
Covered Lines: 7131
Relevant Lines: 9285
💛 - Coveralls

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 82595

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 560 unchanged lines in 37 files lost coverage.
  • Overall coverage increased (+0.8%) to 78.229%
Files with Coverage Reduction New Missed Lines %
/libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/ConfigurationChannelProvider.cs 1 66.67%
/libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/ServiceCollectionExtensions.cs 1 95.24%
/libraries/Microsoft.Bot.Builder.AI.LUIS/Generator/Age.cs 1 66.67%
/libraries/Microsoft.Bot.Builder.AI.LUIS/Generator/Money.cs 1 66.67%
/libraries/Microsoft.Bot.Builder.AI.LUIS/Generator/Temperature.cs 1 66.67%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/ChoicePrompt.cs 1 80.0%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/DatetimePrompt.cs 1 84.21%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/NumberPrompt.cs 1 98.51%
/libraries/Microsoft.Bot.Builder.Dialogs/Prompts/PromptRecognizerResult.cs 1 83.33%
/libraries/Microsoft.Bot.Builder.Dialogs/Dialog.cs 2 94.74%
Totals Coverage Status
Change from base Build 82147: 0.8%
Covered Lines: 5088
Relevant Lines: 6504
💛 - Coveralls

@fuselabs
Copy link
Collaborator

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll compared against version 4.5.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll compared against version 4.5.3

@garypretty garypretty merged commit b2dec6b into master Oct 10, 2019
@cleemullins cleemullins deleted the gary/cosmosDbPartitionedFeedback branch October 24, 2019 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j82w j82w j82w left review comments

@christopheranderson christopheranderson christopheranderson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@garypretty @mdrichardson @coveralls @fuselabs @christopheranderson @j82w