-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Akka.Discovery #4599
Akka.Discovery #4599
Conversation
<PropertyGroup> | ||
<AssemblyTitle>Akka.Discovery.Tests</AssemblyTitle> | ||
<TargetFrameworks>$(NetFrameworkTestVersion);$(NetCoreTestVersion)</TargetFrameworks> | ||
<LangVersion>8</LangVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is fine for new projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that should be fine as long as we don't use default interfaces - which aren't binary backwards compatible
public override Task<Resolved> Lookup(Lookup query, TimeSpan resolveTimeout) => | ||
Resolve(_methods, query, resolveTimeout); | ||
|
||
private async Task<Resolved> Resolve(IReadOnlyCollection<(string, ServiceDiscovery)> sds, Lookup query, TimeSpan resolveTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve
is recursive. I think its fine, but it's open for improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone runs into an issue with it, they we can always change it later
<TargetFramework>$(NetStandardLibVersion)</TargetFramework> | ||
<PackageTags>$(AkkaPackageTags)</PackageTags> | ||
<GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
<LangVersion>8</LangVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this is fine for new projects.
.ToDictionary(pair => pair.Key, pair => | ||
{ | ||
var (serviceName, full) = pair; | ||
var endpoints = full.GetStringList("endpoints"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing GetConfigList
, not currently supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arkatufus is this something we need to add to the HOCON spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've linked an issue on the HOCON repo to this
e309c8a
to
d084bde
Compare
@ismaelhamed looks like you're off to a great start! I'll do a deeper review soon - but do we need to wait until the DNS discovery is available before we consider merging this? Just want to know if I should wait for that before diving in. |
Since both Also, I understand this new module will be released as a |
d084bde
to
b975d95
Compare
Going to review this today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes that I documented - but none worth blocking the merge of this PR. I'll send in a separate PR to add the beta tag for Akka.Discovery so we can include this as part of Akka.NET v1.4.12
@@ -0,0 +1,110 @@ | |||
//----------------------------------------------------------------------- | |||
// <copyright file="Lease.cs" company="Akka.NET Project"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File name is technically wrong here
} | ||
|
||
[InternalApi] | ||
public class AggregateServiceDiscovery : ServiceDiscovery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
<PackageTags>$(AkkaPackageTags)</PackageTags> | ||
<GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
<LangVersion>8</LangVersion> | ||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add a tag here to mark this as a pre-release module
|
||
namespace Akka.Discovery | ||
{ | ||
public class Discovery : IExtension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably have an XML-DOC comment here
@ismaelhamed could you open an issue describing the Akka.IO DNS changes that we need to make in order to support DNS-based service discovery? We should start planning for those. |
Was DNS discovery ever implemented, and if not, is there an expected timeline and/or an issue available to track the progress (tried searching but didn't find it). Thanks! |
Akka Discovery provides an interface around various ways of locating services. The built in methods are:
Current known limitation
Endpoints should be defined as a list of configs:
But AFAIK this is not possible with current Hocon implementation, at least unless #293 is implemented. So, in the mean time I have resorted to define endpoints as a lists of strings (much like we do with
seed-nodes
):