search

OrleansContrib / OrleansTestKit

Unit Test Toolkit for Microsoft Orleans
http://dotnet.github.io/orleans
MIT License
78 stars 43 forks source link

Support for Orleans 7.0 #124

Closed Badabunga closed 1 year ago

Badabunga commented 1 year ago

Sorry for the delayed Pull Request but we had plenty to do. Still we found the Time to get a functional Version which supports Orleans 7.0. As discussed in the Issue #123, we wanted to contribute :

There are some kind of breaking Changes which revolves around the Reminder functionality. There is a Singleton Instance within the Orleans.Grain implementation which holds the Reminder Context of a local Silo. Which will be used to set the Grain Reminder Context. Therefore we needed to add some kind of Context handling to the Test kit to avoid race conditions and thread locks.

If you want to look for a sample you should be able to review it in the Reminder Unit Tests. All other cases will use the same API as before and is already testet with your Unit Tests and ours (250+).

Please reach out to me if something is odd to you.

Thank you and also thank you @mayrmax for the Support

bbehrens commented 1 year ago

@Badabunga Well done sir!

bbehrens commented 1 year ago

Singleton strikes again!

@Badabunga There is a Singleton Instance within the Orleans.Grain implementation which holds the Reminder Context of a local Silo.

ReubenBond commented 1 year ago

There is a Singleton Instance within the Orleans.Grain implementation which holds the Reminder Context of a local Silo.

I'm a little confused about this, what property/field on Grain are you referring to here?

seniorquico commented 1 year ago

I'm a little confused about this, what property/field on Grain are you referring to here?

I'm just taking my first pass over reading this... but I think it's a reference to setting the thread-local IGrainContext instance (this change appears to set it via reflection). Does the new Reminder service depend on accessing IGrainContext through the thread-static?

Badabunga commented 1 year ago

There is a Singleton Instance within the Orleans.Grain implementation which holds the Reminder Context of a local Silo.

I'm a little confused about this, what property/field on Grain are you referring to here?

@ReubenBond : Within the GrainReminderExtensions, (which will be called when the Grain calls the RegisterOrUpdateReminder Method), there is a singleton instance which is called RuntimeContext. This instance will be used to fetch the runtime context to register the grain within the ReminderRegistry as you can see here.

Sadly this instance can not be swapped out because it will be called directly without DI. That's why I needed to create a handling around this Instance because the Unit Tests, which may run parallel, will lead to a race condition where one unit test will override the context of another. Also the RuntimeContext Instance is marked as internal and static which is why I had to do some reflection "magic" to access the Class and swap out the Runtime Context which lives within the internal singleton.

Register Timer on the other hand has a direct implementation within the abstract Grain class and will use the RuntimeContext which is passed during activation of the Grain. Which is why it doesn't need a special handling

jkonecki commented 1 year ago

FIrst of all, many thanks to everyone involved in this PR! Your work is much appreciated.

I'm currently in process of upgrading one of my projects (non-poco grains, reminders, streams) and I wonder if it would be possible to release a pre-release version of OrleansTestKit nuget package. I'm happy to report any issues and don't mind if not everything is working yet.

seniorquico commented 1 year ago

I need to make some tweaks to the automated build scripts... I'll have some free time from work a bit later this week. I'll get a beta build release to NuGet for testing.

Badabunga commented 1 year ago

Just for my Information:
What is the Plan now ? Will this be reviewed or are we going to release the preview Version and test this ?

seniorquico commented 1 year ago

Yes, I think we can at least move forward and make a prerelease push to NuGet for those looking to test.

The single-threaded bottleneck in the reminder is a concern. I was hoping to push this into a discussion on the Orleans project to see if there are any alternative approaches we should consider. But otherwise, the changes look pretty straightforward.

Badabunga commented 1 year ago

Yeah I see your point, that's why I stopped the implementation in the middle because this is way too hacky. But I didn't see another approach to this because of the way it's implemented. Maybe we have some luck on our side and @ReubenBond may have an alternative approach to this

seniorquico commented 1 year ago

that's why I stopped the implementation in the middle

@Badabunga I haven't yet submitted this to the GitHub CI runner for build/test. Is there still outstanding work to be done to finish porting it to Orleans 7? If yes, I might suggest we get that incorporated first. Then I can make the prerelease on NuGet.

Badabunga commented 1 year ago

that's why I stopped the implementation in the middle

@Badabunga I haven't yet submitted this to the GitHub CI runner for build/test. Is there still outstanding work to be done to finish porting it to Orleans 7? If yes, I might suggest we get that incorporated first. Then I can make the prerelease on NuGet.

No it is done , what I meant is the delay between the issue and making the pull request. There is no outstanding work left for the port and should work out of the box.

SamEmber commented 1 year ago

that's why I stopped the implementation in the middle

@Badabunga I haven't yet submitted this to the GitHub CI runner for build/test. Is there still outstanding work to be done to finish porting it to Orleans 7? If yes, I might suggest we get that incorporated first. Then I can make the prerelease on NuGet.

No it is done , what I meant is the delay between the issue and making the pull request. There is no outstanding work left for the port and should work out of the box.

In that case can we please publish a pre release package for this?

palpha commented 1 year ago

Would love to see a pre-release package. We have an extensive set of tests based on OrleansTestKit, and would be happy to help verify that there are no unexpected issues :).

Romanx commented 1 year ago

Same as @palpha happy to trial a pre-release of this if we can get one pushed up to nuget

SamEmber commented 1 year ago

@seniorquico Can we publish a prerelease of this please, its a blocker for our Orleans 7 upgrade

Romanx commented 1 year ago

Hey @seniorquico, I can imagine you're pretty busy. I wonder if theres any way to give one of the people here maintainer access so they can help with some of the management load of the project? I'd be happy to invest some time to help and hopefully others would too.

Zoney commented 1 year ago

Great work! Also happy to contribute here with testing and potensial bug fixes.

seniorquico commented 1 year ago

Hey, all. Apologies for the delays. I'm back, and I'll have some free time this weekend. I'll publish both the latest master branch commit as a release (there's still a couple of pending changes that haven't yet been released for 3.x) and the orleans-7 branch as a prerelease to NuGet.