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

feat: Add the ability to compare existing index definitions to the projected one for a type #479

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

abbottdev
Copy link
Contributor

@abbottdev abbottdev commented Sep 2, 2024

Hi @slorello89,

This PR adds support to be able to have a VERY simple mechanism for the consumer of the library to determine if they need to drop/recreate a given index.

I foresee people using this method like this pseudocode Index drop/create is async I believe, so the consumer would need to do their own polling to wait for the index to properly delete first but:

var provider = new RedisConnectionProvider("redis://localhost:6379");
var definition = provider.Connection.GetIndexInfo(typeof(Customer));

if (definition.IndexDefinitionEquals(typeof(Customer)) == false)
{
    provider.Connection.DropIndex(typeof(Customer));
}

provider.Connection.CreateIndex(typeof(Customer));

Please forgive me that I've not raised an issue for this beforehand - I'm hoping this PR makes enough sense for us to discuss here.

Rather than making all the internals of RedisIndex visible, I've added an extension method that can be used to compare index definitions.

Happy to discuss implementation/naming/test coverage but figure it's easier to discuss something visual.

EDIT: First run of the action failed, looks like ubuntu-latest has moved on to use docker compose over docker-compose (v2) now.

@abbottdev
Copy link
Contributor Author

Bump @slorello89. Hope all's OK - not seen any issue activity on this repo for 2 months and this PR has been outstanding for a bit - any chance we can give the Redis .NET repo some love? ❤️

cc @benoitdion

@obradl
Copy link

obradl commented Sep 22, 2024

I support this functionality, it is much needed. I really enjoy Redis OM for .NET so far and this would be a nice improvement imo.

Copy link
Member

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

@abbottdev - I think the only thing this really needs is to be able to answer this question directly from the connection. e.g. IsModelCurrent

provider.Connection.IsModelCurrent(typeof(Customer));

that way the library just takes care of everything for you without you having to fetch the index definition and then serialize the model yourself.

WDYT?

@slorello89 slorello89 linked an issue Sep 25, 2024 that may be closed by this pull request
@VagyokC4
Copy link

VagyokC4 commented Oct 3, 2024

@abbottdev - I think the only thing this really needs is to be able to answer this question directly from the connection. e.g. IsModelCurrent

provider.Connection.IsModelCurrent(typeof(Customer));

that way the library just takes care of everything for you without you having to fetch the index definition and then serialize the model yourself.

WDYT?

Maybe even a method that keeps the index up to date

provider.Connection.UpdateIndex(typeof(Customer));
await provider.Connection.UpdateIndexAsync(typeof(Customer));

using the pattern below:

var provider = new RedisConnectionProvider("redis://localhost:6379");
var definition = provider.Connection.GetIndexInfo(typeof(Customer));

if (definition.IndexDefinitionEquals(typeof(Customer)) == false)
{
    provider.Connection.DropIndex(typeof(Customer));
}

provider.Connection.CreateIndex(typeof(Customer));

@slorello89
Copy link
Member

@VagyokC4 my big hangup with that approach is that I'm concerned about any implicit index dropping. Perhaps I'm overly cautious, but the drop / create has some implicit downtime (as the index rebuilds), so I'd rather not hide the drop.

Copy link
Member

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

@abbottdev - thanks for submitting this! I hope you don't mind, I pushed an update to add a method directly to the connection to check if the indexes match, I think it's good to go now, just awiting CI/CD

@slorello89 slorello89 merged commit 93e9fd5 into redis:main Oct 4, 2024
1 check passed
@VagyokC4
Copy link

VagyokC4 commented Oct 5, 2024

@VagyokC4 my big hangup with that approach is that I'm concerned about any implicit index dropping. Perhaps I'm overly cautious, but the drop / create has some implicit downtime (as the index rebuilds), so I'd rather not hide the drop.

I thought I saw somewhere there was an update index which doesn't drop it but merely patches it?

@abbottdev
Copy link
Contributor Author

@slorello89 all good from me - apologies I've been on vacation for the last 2 weeks. Happy with another implementation/sig just wanted to get the ball rolling with the initial.

@abbottdev abbottdev deleted the feature/add-indicies branch October 14, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database migration questions
4 participants