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

Minimal Service Fabric integration #2120

Merged

Conversation

ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Sep 6, 2016

In contrast to #2070 (Deep Service Fabric Support) and the related issue #1059, this implements minimal Service Fabric support.

This is a replacement for OrleansContrib/OrleansFabricSilo and introduces a new NuGet package,
Microsoft.Orleans.ServiceFabric, which is referenced by both the client and silo.

A sample project is also included in its own solution. Note, however, that the sample relies on a non-existent version of the nuget packages, so either I can split the samples into a separate PR and wait unitl NuGets are published, or we can determine when this will ship and I can stick those versions in the samples.

Please let me know if you have any questions.

@ReubenBond ReubenBond added this to the Service Fabric milestone Sep 6, 2016
@ReubenBond
Copy link
Member Author

I'd be happy to remove code which sets DeploymentId - it's unnecessary. When deeper integration is done, there will be other ways to configure the target SF service on the client.

@sergeybykov
Copy link
Contributor

There's no easy way to test this, right?

@galvesribeiro
Copy link
Member

So this is just the Stateless service support, right?

@ReubenBond
Copy link
Member Author

@sergeybykov I investigated a little bit and no, there is no easy way to write automated tests for this without writing abstractions for various SF classes. In this case, where the support is very rudimentary, I don't see much value in going to those lengths. If you prefer, I'm happy to try to mock/abstract the SF bits.

@galvesribeiro yes, you're right, this is essentially a port of the OrleansFabricSilo repo

@sergeybykov
Copy link
Contributor

In this case, where the support is very rudimentary, I don't see much value in going to those lengths.

I agree.

If you prefer, I'm happy to try to mock/abstract the SF bits.

If it's relatively easy to do.

"dependencies": {
"Microsoft.Extensions.DependencyInjection.Abstractions": "1.0.0",
"Microsoft.ServiceFabric.Services": "2.1.163",
"Newtonsoft.Json": "7.0.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove transitive dependencies from other projects (DI and Json)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thanks

@ReubenBond
Copy link
Member Author

I'm removing some of the code, push incoming.

@ReubenBond
Copy link
Member Author

ReubenBond commented Sep 7, 2016

The latest revision upgrades the packages to v1.3.0 and removes the code used to detect the IP address, using code already in Orleans for that. I also dropped the DeploymentId stuff, since it's not needed.

@ReubenBond ReubenBond force-pushed the feature-minimal-service-fabric branch 2 times, most recently from 650ff03 to 5f7c5fb Compare September 7, 2016 03:25
@ReubenBond
Copy link
Member Author

ReubenBond commented Sep 7, 2016

I simplified the code further. I'm happy with it now. OpenAsync is the only noteworthy method.

@ReubenBond
Copy link
Member Author

Added basic tests & improved error handling: now errors should occur on construction rather than when opening the listener. I believe this PR is ready for review now (I know, it should be ready when I first submit it - apologies for the many revisions.)

@ReubenBond
Copy link
Member Author

Is anyone able to review this for inclusion into 1.3.0? It does not touch any core Orleans code, it's purely new code and it's very simple.
cc @onionhammer, @miker1423, @kwal, @moswald, since you have used OrleansContrib/OrleansFabricSilo and this is slated to replace it.

@galvesribeiro
Copy link
Member

LGTM

{
"dependencies": {
"Microsoft.ServiceFabric.Services": "2.1.163",
"Newtonsoft.Json": "7.0.1"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is no longer a transitive dependency since it's being used to return the endpoints to SF

@kwal
Copy link

kwal commented Sep 10, 2016

👍

@miker1423
Copy link
Contributor

I'll test it. Great job @ReubenBond!

@moswald
Copy link

moswald commented Sep 11, 2016

The only comment I have is that if you're going to be adding new projects, then I would prefer to see project.json instead of packages.config. Otherwise, it all looks good to me.

@ReubenBond
Copy link
Member Author

Thanks all :)
@moswald the Samples projects have packages.config just because VS added them that way, but the main package uses project.json instead. Do you think the sample should be changed?

@onionhammer
Copy link
Contributor

Please dont convert to project.json; we don't know what the future of that format is going to be, do we?

@moswald
Copy link

moswald commented Sep 11, 2016

The Nuget team has replaced packages.config with project.json. This is not going away. The Asp.Net team tried to replace the .csproj-based system by extending project.json (with nearly-empty .xproj files), but there is just too much tooling around the xml-based system for it to be replaced outright. They are working on moving their "extra" project.json stuff back into a slimmer, more agile .csproj.

@moswald
Copy link

moswald commented Sep 11, 2016

To answer your question Reuben, yeah, I'd prefer to see all projects with project.json support. It's so much easier to edit with the cascading dependency support. This really helps new users (the ones who will be looking at the samples) figure out what's important.

@onionhammer
Copy link
Contributor

I don't know the statistics for this, but I suspect the majority (by a large margin) of Orleans users are using Visual Studio, not VS Code. Orleans does not even currently support dotnet core. I see now reason to use anything 'ahead' of what Orleans itself uses. Wait for the project.json/xproj tooling to go beyond preview/alpha.

@moswald
Copy link

moswald commented Sep 11, 2016

I think you misunderstood me. The combination of csproj and project.json is permanent.

@jdom
Copy link
Member

jdom commented Sep 11, 2016

The reason samples use packages.config is because they can be opened even without the new tooling. It's OK to have Orleans contributors be on whatever tool we find more useful (especially since these tools are free for OSS projects), but we should not mandate that on users of Orleans that want to check out the samples

@moswald
Copy link

moswald commented Sep 11, 2016

There seems to be some misconception about project.json: it's not a part of the dotnet core stuff. It's the preferred file format for Nuget 3.x.

However, we're kind of getting off topic here. I don't really care -- I'm a consumer here, not a contributor and didn't mean to derail anything.

In short, 👍 to the PR. :)

@jdom
Copy link
Member

jdom commented Sep 11, 2016

Yup, I'm aware of it. It still requires nuget v3.5 and VS2015, where all of our samples can run in VS2013 (or even 2012 I believe).
Also minor thing: but since dotnet core is dropping project.json, nuget will probably start using packages.json instead (no big deal though, as it will be the same structure). That probably does not mean that the current file name will stop working in the next nuget release, but it will not be the default any more

@ReubenBond
Copy link
Member Author

Fixed the NuGet package headers, thanks @jdom 👍

@sergeybykov sergeybykov merged commit 8118380 into dotnet:master Sep 14, 2016
@ReubenBond ReubenBond deleted the feature-minimal-service-fabric branch September 22, 2016 06:29
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants