Improve perf of token cleanup #103
Conversation
Use entity framework extended to run the token clean up process. This pevent the cleanup code pulling the data back before deleting it. Add an index to the expiry field to improve performance of selection.
|
Hi @feanz, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@feanz, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| @@ -136,6 +138,8 @@ public static void ConfigureConsents(this DbModelBuilder modelBuilder, string sc | |||
| public static void ConfigureTokens(this DbModelBuilder modelBuilder, string schema) | |||
| { | |||
| modelBuilder.Entity<Token>().ToTable(EfConstants.TableNames.Token, schema); | |||
| modelBuilder.Entity<Token>().Property(x => x.Expiry) | |||
| .HasColumnAnnotation("Index", new IndexAnnotation(new IndexAttribute("Expiry") { IsUnique = false})); | |||
There was a problem hiding this comment.
So I guess this is technically a schema change? Well, an index... that's schema-ish, right? :)
There was a problem hiding this comment.
Also, then, what do migrations look like -- do they know how to include this?
There was a problem hiding this comment.
Yep this is defo a schema change. For migrations it depends how consumers are using this package. When I pulled in IdentityServer3.EntityFramework I explicitly created migrations based on the model. You end up with different migrations sets for
- ScopeConfigurationDbContext
- ClientConfigurationDbContext
- OperationalDbContext
But consumers could setup auto migrations which means ef will run this for you.
There was a problem hiding this comment.
Ok, so I wonder how to go about releasing this. Do we do a point release (e.g. 2.6.0) or do we wait for IdSvr release/build 3.0.0 which we know will introduce schema changes. I think I'd rather wait for IdSvr release/build 3.0.0.
There was a problem hiding this comment.
Yeah I think that makes sense pull it in in three. As long as there is a current issue that allows users to find the details about then it's not a massive issue. You can add the index yourself.
|
Just got around to this now -- I like the idea and sorry for the delay. |
ghost
removed
the
resolves #89
Use entity framework extended to run the token clean up process. This
pevent the cleanup code pulling the data back before deleting it.
Add an index to the expiry field to improve performance of selection.
This current solution does not include @tbarcelon proposed plan to move the clustered index to expiry. It just adds an extra non clustered index.
Note: I've included a test that I would remove before merging this code it just so you can run it quickly run it to check it works.
I've profiled the sql that gets executed by entityframework extended.
Included execution plan
Less than ideal compared to the alternative but probably good enough.