Sessions Storage for CookieStickySessions #1405
Conversation
EliSyrota
changed the title
raman-m
added
feature
Use File-scoped namespace declaration
Use File-scoped namespace declaration
Remove and Sort Usings
|
Hi @EliSyrota ! The feature branch (develop) has been rebased onto ThreeMammals:develop! |
raman-m
requested review from
binarymash,
jps,
geffzhang,
RaynaldM,
philproctor,
anktsrkr,
sharpn,
abooraja,
wast,
yangzhongke and
thiagoloureiro
There was a problem hiding this comment.
Major issues are
- No unit tests for newly added
InMemoryStickySessionStorageclass - No new unit tests in
CookieStickySessionsTests, but behavior ofCookieStickySessionsclass has changed. New parameter for the constructor should be tested. - No tests for
CookieStickySessionsCreator
I believe, current design can be enhanced a bit, to allow to developer not implementing ILoadBalancerCreator interface, but simply replace InMemoryStickySessionStorage class by custom one in DI-container for IStickySessionStorage interface.
There was a problem hiding this comment.
This new class should be covered by unit tests!
| @@ -41,7 +41,8 @@ public CookieStickySessionsTests() | |||
| _bus = new FakeBus<StickySession>(); | |||
| _loadBalancer = new Mock<ILoadBalancer>(); | |||
| _defaultExpiryInMs = 0; | |||
| _stickySessions = new CookieStickySessions(_loadBalancer.Object, "sessionid", _defaultExpiryInMs, _bus); | |||
| var sessionStorage = new InMemoryStickySessionStorage(); | |||
There was a problem hiding this comment.
You are in unit tests! You haven't & shouldn't create concrete instance of the class!
You need to create mocked IStickySessionStorage object for the constructor on the next line.
| var sessionStorage = new InMemoryStickySessionStorage(); | |
| var sessionStorage = new Mock<IStickySessionStorage>(); | |
| // setup the mock |
| @@ -14,8 +15,9 @@ public Response<ILoadBalancer> Create(DownstreamRoute route, IServiceDiscoveryPr | |||
| { | |||
| var loadBalancer = new RoundRobin(async () => await serviceProvider.Get()); | |||
| var bus = new InMemoryBus<StickySession>(); | |||
| var sessionStorage = new InMemoryStickySessionStorage(); | |||
There was a problem hiding this comment.
Is it possible to inject this default concrete class from DI-container, and/or via extra argument?
Because this method is responsible for default implementations injection, we need to forward a IStickySessionStorage object to CookieStickySessions constructor.
substitute CookieStickySessionsCreator to inject into CookieStickySessions own storage mechanism
Because this class (creator) is managed by DI-container, we have to add InMemoryStickySessionStorage class to DI-container too, and inject it via constructor. This is the simplest solution!
Finally, developer should not implement new creator inheriting the ILoadBalancerCreator interface.
P.S. See previous line 17. Consider moving of InMemoryBus<StickySession> object to DI-container too while refactoring this class.
| @@ -0,0 +1,10 @@ | |||
| namespace Ocelot.LoadBalancer.LoadBalancers; | |||
|
|
|||
| public interface IStickySessionStorage | |||
There was a problem hiding this comment.
Well... In PR you said:
- make a public interface for session storage so users can implement their own mechanism of storing user sessions such as distributed sessions storage etc.
Based on the current code I can say, the interface object is not added to DI-container.
| private readonly IBus<StickySession> _bus; | ||
| private readonly object _lock = new(); | ||
|
|
||
| public CookieStickySessions(ILoadBalancer loadBalancer, string key, int keyExpiryInMs, IBus<StickySession> bus) | ||
| public CookieStickySessions(ILoadBalancer loadBalancer, string key, int keyExpiryInMs, IBus<StickySession> bus, IStickySessionStorage storage) |
There was a problem hiding this comment.
substitute CookieStickySessionsCreator to inject into CookieStickySessions own storage mechanism
Because new added interface object can be injected by this creator, we need to explain to developer how to implement custom creator using ILoadBalancerCreator interface.
So, new paragraph in docs is required.
Problem to solve
The current implementation of CookieStickySessions contains ConcurrentDictionary<string, StickySession> _stored where all the sessions are stored.
We faced a situation when we need to have a few Ocelot instances on-premise of the customer
and we have some services that enforces us to use CookieStickySessions for our Load Balancer (https://ocelot.readthedocs.io/en/latest/features/loadbalancer.html#cookiestickysessions).
Having several instances of Ocelot push us to add the ability to use CookieStickySessions for LoadBalancing across these several Ocelot instances.
To do so we need to have an ability to inject our own session storage implementation for CookieStickySessions
New Feature
Proposed Changes
To do so, we need to: