[GitHub] [qpid-broker-j] mklaca commented on pull request #76: QPID-8484: [Broker-J] Reimplementation of the limit number of connections per user

[GitBox <gi...@apache.org>]

mklaca commented on pull request #76:
URL: https://github.com/apache/qpid-broker-j/pull/76#issuecomment-800155282


   Hi Alex,
   
   > I think that creation of design document (we can use qpid confluence) for this new feature (before jumping into implementation) would benefit us both and could save us a lot of time. it is still not to late to do it.
   
   I don't have rights to create a new or edit any qpid confluence page.
   
   > IMHO functionality to check limits should live on ConnectionLimitProviders including registering and de-registering connections. On connection registration/deregistration the VH simply needs to iterate through its own and broker ConnectionLimitProviders and call corresponding methods (register/deregister) to verify limits. The VH should not call ConnectionLimiter and keep a reference to it. Instead, VH should operate with ConnectionLimitProviders.
   
   If the borker ConnectionLimitProvider kept the ConnectionLimiter then the connection counters inside the limiter would be shared amoung multiple VirtualHosts. The VirtualHosts would effect each other, but it is stated in the documentation - [Java Broker concepts](http://qpid.apache.org/releases/qpid-broker-j-8.0.3/book/Java-Broker-Concepts-Virtualhosts.html) that:
   _Virtualhosts are independent; the messaging that goes on within one virtualhost is independent of any messaging that goes on in another virtualhost._
   Hence, I proposed the ConnectionLimitProvider as a factory that builds a new independent limiter, which is provided to a VirtualHost.
   
   VirtualHost can not simple iterate through the limit providers or limiters because the consistency of the connection counters is needed. Let's suppose that the VirtualHost has multiple limit providers. If any of the providers/limiters fails to register connection then all have to fail. The connection has to be registered everywhere or nowhere and so the co-ordination is needed. Hence, the virtual host needs a master limiter that co-ordinates the update of the connection counters.
   
   There are concurrency issues with simple iteration. The working thread needs an exclusive access to all connection counters of the user to check and update all counters atomically.
   
   I see three approaches:
   1. One connection limiter.
   2. Master limiter that co-ordinates all limiters.
   3. Chain of responsibility, when the limiter itself call the next limiter.
   
   I have to point out that the functionality of the multiple limiters has not been requested yet.
   
   > IMHO a special runtime exception can be thrown when any of the limits is breached this type of exception can be caught in the protocol IO layers and connection can be closed accordingly.
   
   I have introduced a new ConnectionLimitException but it looks like a checked exception to me.
   
   > I would prefer to use json/yml over bespoke format. That should reduce implementation code.
   
   I want the file based configuration that is backward compatible with existing ACL rule file. Therefore, I had introduced a ACL like bespoke format and so it is not required to generate a new configuration file in different format. The file based configuration can point to the former ACL file.
   
   > The limit rules can be simplified by removing "Blocked". The user simply can create a rule for ALL where connection limit (frequency limit) is 0.
   
   Yes, it can be but 
   
   - Declaring an user as blocked is more friendly and understandable.
   - Setting "count_limit=0" could be confusing because the value 0 has meaning "no limit" in many other situations. I don't like the idea of the "magic" constant.
   - The blocked user is handled in different way as unblocked user, a connection opened by blocked user can be rejected immediately and any counter is not needed.
   
   > A frequency value can be set as a number of connections per unit of time in the rules. For example, 1/s, 2/m,100/h, 1000/d. I think that would make the rules simpler for the end user.
   
   Yes it is transparent for end user, I have implemented it but the feature complicates evaluation of the rules a lot.
   
   Let suppose that user is a part of the groups gA, gB, gC. Each group has frequency limit:
   gA: frequency limit = 10
   gB: frequency limit = 5
   gC: frequency limit = 15
   I can easily pick up the most restrictive limit 5 because all limits have the same time period and I can easily merge all limits into single effective limit.
   
   The frequency limit 1/s is not the same as 60/m, they looks similar but 60/m allows a short peak of new connections, 1/s does not. If multiple rules could have different time period then I can not pick up the most restrictive limit and the user can have multiple frequency limits with different frequency periods.
   
   > The reject reason can be logged as part of connection close operational logs. Thus, new message resources are not really required. We can delete those and all corresponding logging functionality.
   
   We have received complains and our support team queries the logging of the connection limits. Rejecting a new connection because of the breaking the limits is not a regular close operation.
   
   We need:
   - Log the limit when a new connection is rejected.
   - Log current value of the counter when a new connection is accepted. This should be optional.
   - Connection limit messages should be easy to distinguish and filter out.
   
   > A DeleteTask for every identity should be registered on the connection to free the slot.
   
   I can not use DeleteTask because it does not ensure release of the connection slot when any of the delete tasks fails.
   Suppose that the connection has 3 delete task, if first task fails then the second and third task are not executed. Hence, the connection counter remains none zero for ever.
   The de-registering the connection from VirtualHost is not delete task because of the same reason.
   
   Regards,
   Marek


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org