Closed JorritSalverda closed 7 years ago
Porting the project to .NET Core is done. Since most of the stuff was moved around, the PR shows almost all the code in the repository, I don't know how practical it is to properly review everything.
Main things done:
HystrixMetricsStreamEndpoint
for both frameworks. Also tried to optimize it a bit to create fewer temporary strings and byte arrays, but rather write directly to the output stream, so please review that class.
(In the ASP.NET Core version I occasionally get some Kestrel error messages (https://github.com/aspnet/KestrelHttpServer/issues/1402) when the connection is closed, but they don't seem to cause actual problems.)HystrixRollingNumberEvent
in order not to have to use reflection. @JorritSalverda please take a look. I don't really like how I changed it, but if the set of enums don't change, I think it's fine.I also created two sample applications, one for ASP.NET and one for ASP.NET Core, but haven't added them to the PR yet, since the packages themselves are not published.
Now the plan is:
In terms of versioning, since this is breaking change, I guess 1.0.0 makes sense?
1.0.0 make sense indeed. Do you really have to push it before you can add the example code though? Elsewhere we have nuspec files that refer to the same version as built by using version="$version$" in the dependencies.
The HystrixRollingNumberEvent doesn't look too nice indeed. Perhaps but some comments behind the enum values to indicate what it is? Can't we run the reflection once and then store it in 2 dictionaires? A IsCounterDictionary and IsMaxUpdaterCounter?
@JorritSalverda From the samples (and the extensions packages) I want to refer to the other libraries as Nuget package references, and not source references. Maybe then I'll push a prerelease Nuget package from all three libraries now, and add the samples too.
HystrixRollingNumberEvent: I wanted to avoid having a Dictionary to make it as fast as possible. I think the ideal solution would be to instead of an enum have an actual data type which can hold this information, sth like this:
public enum EventKind
{
Counter,
MaxUpdater
}
public struct HystrixRollingNumberEvent
{
HystrixRollingNumberEvent(EventKind kind)
{
Kind = kind;
}
public EventKind Kind { get; }
}
public static class HystrixRollingNumberEvents
{
public readonly static HystrixRollingNumberEvent Success = new HystrixRollingNumberEvent(EventKind.Counter);
public readonly static HystrixRollingNumberEvent Failure = new HystrixRollingNumberEvent(EventKind.Counter);
...
public readonly static HystrixRollingNumberEvent CommandMaxActive = new HystrixRollingNumberEvent(EventKind.MaxUpdater);
...
}
I started to do this, but then the rest completley breaks because we depend on the enum values being convertible to ints in order to make an array out of them, so I gave up.
Anyway, I'll change it to the Dictionary approach.
Oh, and I actually wanted to remove the reflection, because that was the only thing preventing us from decreasing the version to netstandard1.3
.
@JorritSalverda Changed the HystrixRollingNumberEvent implementation, now it'll throw an exception if we forget to set up the EventKind for any of the enums, so the unit tests will fail.
Logging is fully commented out currently?