Enable "cleaning" of all dead entries in the membership table #5389
Conversation
| return entriesList.Count(); | ||
| } | ||
|
|
||
| public async Task DeleteDeadMembershipTableEntries(DateTime beforeDate) |
There was a problem hiding this comment.
This should be a DateTimeOffset
| return true; | ||
| }).Ignore(); | ||
| } | ||
| catch (MissingMethodException) |
There was a problem hiding this comment.
Do we need to also catch NotImplementedException?
There was a problem hiding this comment.
I thought that they were caught in the Continue but it seems you are right
There was a problem hiding this comment.
Maybe we should use an async method instead of .ContinueWith().Ignore(), so the error handling is flat
| /// and will delete them | ||
| /// </summary> | ||
| public TimeSpan? CleanupDeadEntriesTimeout { get; set; } = DEFAULT_DELETE_ENTRIES_OLDER_THAN; | ||
| public static readonly TimeSpan? DEFAULT_CLEANUP_DEAD_ENTRIES_TIMEOUT = null; |
There was a problem hiding this comment.
This was confusing to me before reading the code. I think we should rename it to something like DefunctSiloCleanupPeriod and change the comment to:
/// <summary>
/// The duration between membership table cleanup operations. When this period elapses, all defunct silo
// entries older than <see cref="DefunctSiloExpiration" /> are removed. This value is per-silo.
/// </summary>Thoughts? I'm not set on those names, but 'timeout' to me says that the process will be cancelled after that amount of time.
There was a problem hiding this comment.
Yes I like your names better
|
many test failures, unsure why |
|
Made some minor changes. LGTM & will squash merge once tests complete again. |
|
@dotnet-bot test functional |
|
I noticed this is listed as a non-breaking change. If i have a custom membership provider that did not upgrade, wouldn't that break when i upgrade to Orleans 2.3? What would be then considered as breaking change? |
|
@jinhong- If you are compiling your custom membership provider at the same time as you are compiling your silos then you will have to add this method to your interface to compile properly. You can throw If you are referencing via NuGet for example, then it should be fine since it will be catched by the runtime. |
|
@benjaminpetit Okay. So in theory i could use a Membership Provider installed via NuGet package compiled with Orleans 2.2 in the new Orleans 2.3? On another note, i notice this line seemed a little strange to me. |
|
@jinhong- you are right, the condition should be
|
Yes it should work |
|
Why Consul not implemented? // src/Orleans.Clustering.Consul/ConsulBasedMembershipTable.cs
public Task CleanupDefunctSiloEntries(DateTimeOffset beforeDate)
{
throw new NotImplementedException();
} |
|
Because we didn't implemented the logic for Consul yet. But feel free to open a PR with the implementation if you can, I would be happy to review it and merge it! |
Right now, we don't delete old entries in the membership table, and that can be an issue since it will grow forever.
This PR enable deleting old silo entries, and implement it in the azure membership provider. Other providers still need to implement this logic.