Health service #15
Health service #15
Conversation
| /// <exception cref="ArgumentException">If the specified value is an invalid value.</exception> | ||
| /// <returns>The result of the conversion.</returns> | ||
| public static explicit operator HealthStatus(byte value) => | ||
| value <= Unhealthy.value ? new HealthStatus(value) : throw new ArgumentException(nameof(value)); |
There was a problem hiding this comment.
Could we think about InvalidCastException? maybe is more verbose
| /// <summary> | ||
| /// Represents the status of a health check result. | ||
| /// </summary> | ||
| public struct HealthStatus : IEquatable<HealthStatus>, IComparable<HealthStatus> |
There was a problem hiding this comment.
this functionality is really similar than Enum's one. is the size (enum allocate ints (32 bytes)) justification for maintain this code?
There was a problem hiding this comment.
Actually, in .Net an enum is only a numeric value, not a closed set.
src/Health.Service/IHealthService.cs
Outdated
| using System.Threading.Tasks; | ||
|
|
||
| /// <summary> | ||
| /// A health service that contains multiple <see cref="IHealthCheck"/>. |
There was a problem hiding this comment.
There isn't a real dependecy of this abstraction with IHealthCheck.
I'd like to create a more generic comment avoiding mention IHealthCheck
| /// <exception cref="ArgumentException">If the specified value is an invalid value.</exception> | ||
| /// <returns>The result of the conversion.</returns> | ||
| public static explicit operator HealthStatus(int value) => | ||
| value <= Unhealthy.value ? new HealthStatus((byte)value) : throw new ArgumentException(nameof(value)); |
There was a problem hiding this comment.
could be confused when overflow because negative ints? is it a desirable feature?
| /// </summary> | ||
| public TimeSpan TotalDuration { get; } | ||
|
|
||
| private static HealthStatus GetLowerStatus(IEnumerable<HealthStatus> statuses) |
There was a problem hiding this comment.
could be confuse when the overflow of some shorts, int and longs (negative):
see comment: https://github.com/payvision-development/sakikku/pull/15/files#diff-e27231d79f2bbeae67b5011c1aee0cb3R65
There was a problem hiding this comment.
The comparisson is done for HealthStatus types which is a value by itself, so there is not possibility of overflows here.
| new Dictionary<string, ReactiveHealthCheck>(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IHealthCheckFactory Add(string name) |
There was a problem hiding this comment.
this way to include elements in a set could include incosistant checks (not calling for on check elements).
We should check on Build method for only the "good" ones or null exception could be thrown when Build method of ReactiveHealthCheck is called.
There was a problem hiding this comment.
You're absolutelly right
| return this; | ||
| } | ||
|
|
||
| internal IObservable<HealthCheckEntry> Build(IScheduler scheduler) |
There was a problem hiding this comment.
I'm not sure but i think this check will only be executed ones per subscription not in a time base.
Maybe we could clarify this point
There was a problem hiding this comment.
Actually we're building a promise here, which is going to be decorated on the health check set build method. Doing in that way we're going to be able of configure the execution behavior afterwards.
… to a HealthCheckSet among its name.
Resolves #1