[GitHub] [activemq-artemis] gemmellr edited a comment on pull request #3634: ARTEMIS-3365 Broker Balancers

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

gemmellr edited a comment on pull request #3634:
URL: https://github.com/apache/activemq-artemis/pull/3634#issuecomment-894143709


   I dont think the keystore changes should using NO-JIRA in their log, at least some are being made directly for this JIRA so it makes sense for it to be reflected in the commit log.
   
   I think Clebert was looking to avoid creating the redirect handler objects entirely if not needed, which the updated version doesnt appear to. It still looks to affect every connection, but instead of with the throwaway handler, by adding a new field and creating the handler up front, then just possibly not using it. In some ways this is actually worse than previous version.
   
   I think there is a nicer route in-between, but nearer the earlier version 2212d02f206bbdf9916db225e876eef4583c1326 [1], where it simply checks the redirect-to value on the connection first, and only then proceeds to create and use the handler if there was a need. E.g changes from the previous version for AMQP could maybe have been something like this instead (+add similar for other protocols):
   
   ```
   @@ -528,6 +528,5 @@ public class AMQPConnectionContext extends ProtonInitializable implements EventH
    
   -      AMQPRedirectHandler redirectHandler = new AMQPRedirectHandler(protocolManager.getServer(),
   -         connectionCallback, connection, handler.getSASLResult() != null ? handler.getSASLResult().getUser() : null);
   +      final boolean tryRedirect = connectionCallback.getTransportConnection().getRedirectTo() != null;
    
   -      if (redirectHandler.redirect() || !validateConnection(connection)) {
   +      if (tryRedirect && redirect(connection) || !validateConnection(connection)) {
             connection.close();
   @@ -557,2 +556,9 @@ public class AMQPConnectionContext extends ProtonInitializable implements EventH
    
   +   private final boolean redirect(Connection connection) throws Exception {
   +      AMQPRedirectHandler redirectHandler = new AMQPRedirectHandler(protocolManager.getServer(),
   +            connectionCallback, connection, handler.getSASLResult() != null ? handler.getSASLResult().getUser() : null);
   +
   +      return redirectHandler.redirect();
   +   }
   +
       class ScheduleOperator implements UnaryOperator<Future<?>> {
   ```
   
   
   [1]  I still had the the 2212d02f206bbdf9916db225e876eef4583c1326 version locally, I put it at https://github.com/gemmellr/activemq-artemis/tree/balancer-pr3634-old1 in case you should want it.


-- 
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.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

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