OrleansContrib/Orleans.Providers.MongoDB

Orleans 7.2.1 and Reminders

snovak7 opened this issue · 7 comments

I am having trouble booting up the server when I want to use Reminders with MongoDb, Clustering and Storage seem to work just fine.

Exception is unrelated to the MongoDb, but I have noticed that the Database is not created.

I am using the latest nuget package 7.5.0

Keep up the good work.

nesc58 commented

Same here.

The issue is the following:
Orleans.Providers.MongoDB reference Microsoft.Orleans.Reminders in version 7.0.0.
LocalReminderService in 7.0.0 uses the following code inside the Participate method:

await this.QueueTask(Start)
   .WithCancellation(ct, $"Starting ReminderService failed due to timeout {initTimeout}");

The 7.2.1 version of orleans contains a different implementation

await this.QueueTask(Start)
   .WithCancellation($"Starting ReminderService failed due to timeout {initTimeout}", ct);

No idea why microsoft changed the WithCancellation call from 7.0.0 to 7.2.1 but thats why it crash. The method could not longer be found.

There are two ways to solve the problem:

  • You can install Microsoft.Orleans.Reminders explicit in version 7.2.1
  • The maintainer could update the whole project to use Orleans 7.2.1

Why should the WithCancellation call have any impact?

Btw: The golden rules of Open Source. If you want to have fixed something fast, provide a PR ;)

nesc58 commented

The LocalReminderService internally uses this.
When setting up a project using Microsoft.Orleans.Server in 7.2.1 and Orleans.Providers.MongoDB in 7.5.0
Microsoft.Orleans.Reminders will be resolved with 7.0.0.
Something seems to be incompatible between Microsoft.Orleans.Reminders in 7.0.0 and the Microsoft.Orleans.Server (and Microsoft.Orleans.Runtime and so on) 7.2.1.

Take a look here: https://github.com/dotnet/orleans/releases/tag/v7.2.0
They have moved the CancellationTokem arguments to the end.

I will do a PR when I am done with testing. Currently i am unable to execute the tests locally and i have to fix this first
:-)

Thanks @snovak7 for bringing this to our attention and @nesc58 for your work.
I've created a PR to address those changes and also included some improvements to unit/integration tests.
@SebastianStehle if you don't mind having a look when you get a chance.

Also apologies for the delayed response, for some reason notifications went to my spam folder.

PR has been merged, please update dependencies and let me know if the issue is resolved.

Looks good to me, thank you @nesc58 @wassim-k