Rate limit#37
Conversation
merge current code
merge servicediscovery
Merge CircuitBreakerPattern
|
@geffzhang I will check this soon, thanks for the pull request. |
|
@geffzhang Looks really good 👍, maybe we could add an acceptance test that shows the rate limiting is working end to end. I find acceptance tests very useful when refactoring. |
|
@TomPallister yes，acceptance test very useful when refactoring. I have added two test |
TomPallister
left a comment
TomPallister
left a comment
There was a problem hiding this comment.
@geffzhang pull request is very good! Thank you. I have left some comments please check them.
| } | ||
|
|
||
| public class RateLimitRule | ||
| { |
There was a problem hiding this comment.
Can this class be changed so that it's properties are {get; private set;} when constructing an object I prefer that it's properties cannot be changed.
| /// <summary> | ||
| /// Stores the initial access time and the numbers of calls made from that point | ||
| /// </summary> | ||
| public struct RateLimitCounter |
There was a problem hiding this comment.
Can these properties be {get;private set}
|
|
||
| namespace Ocelot.RateLimit | ||
| { | ||
| public class RateLimitHeaders |
There was a problem hiding this comment.
Can these properties be {get;private set}
| public string RetryAfterFrom(DateTime timestamp, RateLimitRule rule) | ||
| { | ||
| var secondsPast = Convert.ToInt32((DateTime.UtcNow - timestamp).TotalSeconds); | ||
| var retryAfter = Convert.ToInt32(rule.PeriodTimespan.Value.TotalSeconds); |
There was a problem hiding this comment.
Maybe check this is not null before operation or change property so it is not nullable.
| } | ||
|
|
||
| return headers; | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
We should probably remove this
|
|
||
| public bool IsWhitelisted(ClientRequestIdentity requestIdentity, RateLimitOptions option) | ||
| { | ||
| if (option.ClientWhitelist != null && option.ClientWhitelist.Contains(requestIdentity.ClientId)) |
There was a problem hiding this comment.
If we set this list always then it cannot be null and we dont need to check here
| @@ -0,0 +1,11 @@ | |||
| namespace Ocelot.RateLimit | |||
| { | |||
| public class ClientRequestIdentity | |||
There was a problem hiding this comment.
Can the properties of this class be {get;private set;}
|
|
||
| namespace Ocelot.AcceptanceTests | ||
| { | ||
| public class ClientRateLimitTests : IDisposable |
There was a problem hiding this comment.
I think we need one more acceptance test that checks if rate limiting is set the first request is not accidentally rate limited. The test should_call_withratelimiting() only checks the last reponse was rate limited. Perhaps we should check each response to make sure it is what we expect.
| _builder.Start(); | ||
| } | ||
|
|
||
| //private void GetApiRateLimait(string url) |
There was a problem hiding this comment.
We should remove commented out code
test/Ocelot.ManualTest/Startup.cs
Outdated
| .WithDictionaryHandle(); | ||
| }; | ||
|
|
||
| services.AddMemoryCache(); |
There was a problem hiding this comment.
If this is required for rate limiting to work maybe we should add it in ServiceCollectionExtensions.AddOcelot
|
OK, changing for the better, consistent coding style |
|
@geffzhang I also think those properties are readonly maybe better(using C# 6 feature). |
|
@xyting 👍 |
|
@geffzhang looks good, feel free to merge! |
Merge Newest code
…imit # Conflicts: # src/Ocelot/Configuration/Builder/ReRouteBuilder.cs # src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs # src/Ocelot/Configuration/QoSOptions.cs # src/Ocelot/Configuration/ReRoute.cs # test/Ocelot.AcceptanceTests/configuration.json
|
@TomPallister I have merged code |
4 participants
implement Request Rate limit, this feature is options