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

Add a blob container client factory for customisation #7849

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Add a blob container client factory for customisation #7849

merged 5 commits into from
Apr 11, 2023

Conversation

Romanx
Copy link
Contributor

@Romanx Romanx commented Jul 11, 2022 •
edited by ReubenBond
Loading

Allows blob containers to be customised based on grainType and grain id. This is wrapped in an interface to allow injection of dependencies and state to be maintained.

Fixes #7831

Microsoft Reviewers: Open in CodeFlow

@ReubenBond ReubenBond force-pushed the feature/blob-storage-container-customisation branch from 79c21b0 to e084964 Compare July 11, 2022 15:17
@Romanx Romanx force-pushed the feature/blob-storage-container-customisation branch from e084964 to 4f397bd Compare July 12, 2022 08:00
@Romanx Romanx marked this pull request as ready for review July 13, 2022 13:07
@ReubenBond ReubenBond force-pushed the feature/blob-storage-container-customisation branch from 4f397bd to 9f2a522 Compare July 13, 2022 20:47
@ReubenBond ReubenBond self-assigned this Jul 13, 2022
ReubenBond
ReubenBond reviewed Jul 15, 2022
View reviewed changes
src/Azure/Orleans.Persistence.AzureStorage/Providers/Storage/AzureBlobStorage.cs Outdated
@@ -189,6 +195,8 @@ public async Task WriteStateAsync<T>(string grainType, GrainReference grainId, I
public async Task ClearStateAsync<T>(string grainType, GrainReference grainId, IGrainState<T> grainState)
{
var blobName = GetBlobName(grainType, grainId);
var container = this.blobContainerFactory.BuildContainerClient(grainType, grainId);
Copy link
Member

Choose a reason for hiding this comment

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

This seems expensive - creating a new container for each call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wasn't wild about this either but I can't think of another way of handling it. The default implementation wouldn't create a new container each call but I guess could

Copy link
Member

Choose a reason for hiding this comment

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

It seems wrong that everyone should have to pay for this, given it serves a very niche purpose. I think it would be better to have a different storage provider, eg a sharded/partitioned storage provider which delegates to another storage provider based on the partition which the grain is in. That also allows sharding across storage accounts (which this would not), so it is more compelling. cc @benjaminpetit (who is out of office currently)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everyone wouldn't have to pay for this apart from the overhead of the method call to the factory.

Copy link
Member

Choose a reason for hiding this comment

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

Ah you're right, my bad.

@Romanx Romanx force-pushed the feature/blob-storage-container-customisation branch 2 times, most recently from 01e3086 to b90f431 Compare July 19, 2022 09:32
@Romanx Romanx force-pushed the feature/blob-storage-container-customisation branch from b90f431 to 0e768be Compare July 26, 2022 08:38
@Romanx
Copy link
Contributor Author

Romanx commented Jul 28, 2022

@ReubenBond is there anything extra you'd like done on this?

@Romanx Romanx force-pushed the feature/blob-storage-container-customisation branch 2 times, most recently from 333248b to a39a426 Compare August 11, 2022 08:16
@ReubenBond ReubenBond force-pushed the feature/blob-storage-container-customisation branch from a39a426 to 29d5fef Compare August 18, 2022 17:50
@ReubenBond
Copy link
Member

ReubenBond commented Aug 18, 2022 •
edited
Loading

LGTM. cc @benjaminpetit for feedback. I made some minor adjustments in the latest commit.

@ReubenBond ReubenBond modified the milestones: .NET 7.0, 4.0.0-preview3 Aug 18, 2022
Alex McAuliffe and others added 5 commits September 13, 2022 11:40
@ReubenBond
Add a blob container client factory for customisation
b1c4796 
Allows blob containers to be customised based on grainType and grain id. This is wrapped in an interface to allow injection of dependencies and state to be maintained.
@ReubenBond
Update src/Azure/Orleans.Persistence.AzureStorage/Providers/Storage/I…
1ffff42 
…BlobContainerFactory.cs
@ReubenBond
Use GrainId since in v4 that's better
f16fa45
@ReubenBond
Build fixes & minor feedback adjustments
9732834
@ReubenBond
Remove redundant code
1f450b2
@ReubenBond ReubenBond force-pushed the feature/blob-storage-container-customisation branch from 1f9bef2 to 1f450b2 Compare September 13, 2022 18:40
@ReubenBond
Copy link
Member

@benjaminpetit PTAL. I recall you had an opinion that this should be implemented using a delegate instead. I wonder how that fits with the InitializeAsync method: should the delegate return a ValueTask<BlobContainerClient>?

@Romanx
Copy link
Contributor Author

Romanx commented Nov 9, 2022

Hi Ruben, congratulations to y'all for shipping Orleans 7.

I was wondering if there was any plans for this since the release has been completed, happy to help make any changes needed on it.

@ReubenBond
Copy link
Member

Hi @Romanx, apologies for the lack of activity here. @benjaminpetit raised some points about using a configurable delegate to select the container instead of a 2-method interface. I'd like to see how that alternative looks in code

@Romanx
Copy link
Contributor Author

Romanx commented Nov 11, 2022

Hey @ReubenBond, no problem at all. @benjaminpetit, Let me know if you need any help getting it going

@Romanx
Copy link
Contributor Author

Romanx commented Jan 5, 2023 •
edited
Loading

@ReubenBond @benjaminpetit, this is becoming a little more pressing for my project as we're moving to Orleans 7.0 at the moment and we'd rather not copy the storage provider into our project.

Any updates on where we are with this, happy to spend some time if you've got ideas on what changes you'd like but no time yourselves?

@ReubenBond
Copy link
Member

ReubenBond commented Jan 11, 2023 •
edited
Loading

cc @benjaminpetit let's come up with a solution/agreement on this tomorrow, if possible

@ReubenBond ReubenBond modified the milestones: 4.0.0-preview3, 7.1.0 Jan 12, 2023
@ReubenBond ReubenBond modified the milestones: 7.1.0, 7.1.1 Feb 22, 2023
@Romanx
Copy link
Contributor Author

Romanx commented Apr 11, 2023

Sorry to nudge again on this one. Is there any kind of ETA on this getting merged? We could bring this code directly into our codebase but then makes us not use the AzureStorage provider which could drift and we would rather not.

Cheers

@benjaminpetit benjaminpetit merged commit b24bcc1 into dotnet:main Apr 11, 2023
@ReubenBond ReubenBond mentioned this pull request Apr 18, 2023
Merged
@Romanx Romanx deleted the feature/blob-storage-container-customisation branch August 3, 2023 15:57
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ReubenBond ReubenBond ReubenBond approved these changes

@benjaminpetit benjaminpetit Awaiting requested review from benjaminpetit

Assignees

@ReubenBond ReubenBond

@benjaminpetit benjaminpetit

Labels
None yet
Projects
None yet
Milestone
7.1.1
Development

Successfully merging this pull request may close these issues.

AzureBlobStorage target container customisation
3 participants
@Romanx @ReubenBond @benjaminpetit