OrleansContrib / Orleans.Providers.MongoDB

A MongoDb implementation of the Orleans Providers: Membership, Storage and Reminders.
MIT License
105 stars 44 forks source link

Orleans 7.2.1 and Reminders #127

Closed snovak7 closed 1 year ago

snovak7 commented 1 year ago

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 1 year ago

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:

SebastianStehle commented 1 year ago

Why should the WithCancellation call have any impact?

SebastianStehle commented 1 year ago

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

nesc58 commented 1 year ago

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 :-)

wassim-k commented 1 year ago

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.

wassim-k commented 1 year ago

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

snovak7 commented 1 year ago

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