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

Incorporate Orleans.Redis providers #8212

Merged
merged 14 commits into from Dec 19, 2022
Merged

Incorporate Orleans.Redis providers #8212

merged 14 commits into from Dec 19, 2022

Conversation

EPinci
Copy link
Contributor

@EPinci EPinci commented Dec 5, 2022

Integrating Redis providers from Contrib repo as per #8207.

  • Created Redis solution folder under Extensions
  • Moved Directory provider under it
  • Added Clustering, Persistence and Reminders providers
  • Bumped StackExchange.Redis dependency to latest version
  • Rewrote test integrating with Tester.Redis
Microsoft Reviewers: Open in CodeFlow

@EPinci EPinci changed the title Integrate Redis providers Incorporate Orleans.Redis providers Dec 5, 2022
@EPinci
Copy link
Contributor Author

EPinci commented Dec 9, 2022

@ReubenBond, @benjaminpetit, I rewrote all the Persistence, Reminders and Clustering test trying to derive from the bases in TesterInternal but I'm not sure I got them right - two have fixes outside the Redis code that need validation, one is still failing.

About the MultipleGrainDirectoryTests:
I found a typo here that should explain why the tests never run for Redis, I made a fix but I take that the same problem applies to the only other Directory provider using it, the Azure one.
On a hunch I would say that now this test could be failing for Azure as well. For some reasons the CI pipeline running on my PR skips the Azure tests so I cannot validate.
The test fails on this call but I have no clue on why.

About the Clustering/CleanupDefunctSiloEntries:
I made a fix in the Redis provider for it but... what is the right way it should behave?

  • The base TesterInteranal class creates a stale dead silo, a stale joining silo and an active silo; expects the cleanup to end up with a single silo (removing both the stales).
  • The former Redis code removed only stale dead silos (the joining one would remain there)
  • The Azure code removes stale !dead silos
  • The consul code removes silos with outdated IAmAliveTime timestamps, regardless of the state
  • The IGrainDirectory interface comments says dead (only) but then the test assumes joining silos to be removed as well

About the Clustering/GetGateways:
I made a fix in the TesterInternal/MembershipTests/MembershipTableTestBase.cs but...

  • The IGrainDirectory interface comments of InsertRow is ambiguous to me: point 3 does not clarify if the method or the caller is supposed to update the version - key is update as in "provide the version.Next()" versus update as in "persist the changed one"
  • In the same MembershipTableTestBase above, L115 repeatedly calls InsertRow with the same version (the first call with an <0,0> version that breaks the Redis test) while, on L154 the same call is made with an "updated" version. Seems inconsistent to me and that is why I made the change above. Curiously, that fixes the Redis test while not breaking other providers tests (per above, cannot test Azure)
  • The code implementation of InsertRow is very differently implemented across providers. More importantly, does seems to me that most of the provider actually implement version consistency check like Redis has - note that I'm not implying that the Redis code I'm merging has to be the correct one...

@benjaminpetit
Copy link
Member

About the MultipleGrainDirectoryTests:

Yes, I can confirm that AzureMultipleGrainDirectoriesTests is failing too...

About the Clustering/CleanupDefunctSiloEntries:

I think it should remove dead and joining silos. Active entries should be voted dead first.

About the Clustering/GetGateways:

I don't think the IMembershipTable (not the IGrainDirectory, I assume it's an error?) should care about the value of TableVersion.Version. Only the etag should be used as a concurrency check. Asking @ReubenBond to confirm

@ReubenBond
Copy link
Member

Only the etag should be used as a concurrency check. Asking @ReubenBond to confirm

Correct, as far as concurrency checks are concerned. If the value read has a version less than the previously read version, though, it will be ignored by the cluster since versions must be monotonically increasing.

@EPinci EPinci marked this pull request as ready for review December 16, 2022 23:50
@ReubenBond
Copy link
Member

Functional are failing because some tests (Azure, Redis) are not being skipped when they don't have a connection string specified

@ReubenBond ReubenBond merged commit d10d804 into dotnet:main Dec 19, 2022
@ReubenBond
Copy link
Member

Fantastic work, @EPinci!

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants