[GitHub] [james-project] chibenwa commented on a diff in pull request #1403: JAMES-3876 Load-balancing flag for Remote Delivery Gateways

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

chibenwa commented on code in PR #1403:
URL: https://github.com/apache/james-project/pull/1403#discussion_r1081137417


##########
server/apps/spring-app/src/main/resources/mailetcontainer.xml:
##########
@@ -354,6 +354,11 @@ Regards, Postmaster XXX.YYY
           <gateway>otherserver.mydomain.com</gateway>
           <gatewayPort>25</gatewayPort>
             -->
+          <!-- in case of multiple gateway round-robin load balancing can bi activated -->
+          <!-- with the loadBalancing flag, default is false -->
+          <!--
+          <loadBalancing>true</loadBalancing>

Review Comment:
   Personnally I do not think this setting is necessary to be added.
   
   That is not an objection on my side, but naturrally I would have been applying it all the time.



##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/remote/delivery/RemoteDeliveryConfiguration.java:
##########
@@ -281,6 +288,14 @@ public Collection<String> getGatewayServer() {
         return gatewayServer;
     }
 
+    public synchronized Collection<String> getGatewayServerRotateList() {
+        if (!gatewayServer.isEmpty() && gatewayServer.size() > 0) {
+            Collections.rotate(gatewayServerRotateList,1);
+            return List.copyOf(gatewayServerRotateList);
+        }
+        return List.copyOf(gatewayServerRotateList);
+    }

Review Comment:
   Here we have a design problem.
   
    -> a Configuration is meant as "immutable" and represent the content of the conf file.
    
    It should be absent of application logic and shouldn't  hold state.
    
    The use of `synchronized` looks like an uneeded bottleneck to me.
    
    I would suggest just copying the collection of gateways and shuffling it for each outgoing email
     -> no state
     -> no synchronization.
     
     Thoughts?



##########
server/apps/spring-app/src/main/resources/mailetcontainer.xml:
##########
@@ -354,6 +354,11 @@ Regards, Postmaster XXX.YYY
           <gateway>otherserver.mydomain.com</gateway>
           <gatewayPort>25</gatewayPort>
             -->
+          <!-- in case of multiple gateway round-robin load balancing can bi activated -->
+          <!-- with the loadBalancing flag, default is false -->

Review Comment:
   ```suggestion
             <!-- with the loadBalancing flag, default is false -->
   ```
   
   I think default can be true: this is definitly a desirable behaviour.



##########
server/apps/spring-app/src/main/resources/mailetcontainer.xml:
##########
@@ -354,6 +354,11 @@ Regards, Postmaster XXX.YYY
           <gateway>otherserver.mydomain.com</gateway>
           <gatewayPort>25</gatewayPort>
             -->
+          <!-- in case of multiple gateway round-robin load balancing can bi activated -->

Review Comment:
   ```suggestion
             <!-- in case of multiple gateway round-robin load balancing can be activated -->
   ```



-- 
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: notifications-unsubscribe@james.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org