[GitHub] [pulsar] eolivelli commented on a diff in pull request #19490: [improve][broker] PIP-192 Use msg property to mark compacted msgs from strategic compaction

["eolivelli (via GitHub)" <gi...@apache.org>]

eolivelli commented on code in PR #19490:
URL: https://github.com/apache/pulsar/pull/19490#discussion_r1104373474


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateCompactionStrategy.java:
##########
@@ -57,33 +54,45 @@ public boolean shouldKeepLeft(ServiceUnitStateData from, ServiceUnitStateData to
         }
 
         if (checkBrokers) {
-            if (prevState == Free && (state == Assigned || state == Owned)) {
-                // Free -> Assigned || Owned broker check
-                return StringUtils.isBlank(to.broker());
-            } else if (prevState == Owned && state == Assigned) {
-                // Owned -> Assigned(transfer) broker check
-                return !StringUtils.equals(from.broker(), to.sourceBroker())
-                        || StringUtils.isBlank(to.broker())
-                        || StringUtils.equals(from.broker(), to.broker());
-            } else if (prevState == Assigned && state == Released) {
-                // Assigned -> Released(transfer) broker check
-                return !StringUtils.equals(from.broker(), to.broker())
-                        || !StringUtils.equals(from.sourceBroker(), to.sourceBroker());
-            } else if (prevState == Released && state == Owned) {
-                // Released -> Owned(transfer) broker check
-                return !StringUtils.equals(from.broker(), to.broker())
-                        || !StringUtils.equals(from.sourceBroker(), to.sourceBroker());
-            } else if (prevState == Assigned && state == Owned) {
-                // Assigned -> Owned broker check
-                return !StringUtils.equals(from.broker(), to.broker())
-                        || !StringUtils.equals(from.sourceBroker(), to.sourceBroker());
-            } else if (prevState == Owned && state == Splitting) {
-                // Owned -> Splitting broker check
-                return !StringUtils.equals(from.broker(), to.broker());
+            switch (prevState) {
+                case Free:
+                    switch (state) {
+                        case Assigned:
+                        case Owned:
+                            return isNotBlank(to.sourceBroker());
+                    }
+                case Owned:
+                    switch (state) {
+                        case Assigned:
+                            return invalidTransfer(from, to);
+                        case Splitting:
+                            return !from.broker().equals(to.broker());
+                    }
+                case Assigned:
+                    switch (state) {
+                        case Released:
+                        case Owned:
+                            return notEquals(from, to);
+                    }
+                case Released:
+                    switch (state) {
+                        case Owned:
+                            return notEquals(from, to);
+                    }
+                case Splitting:
+                    return false;
             }
         }
-
         return false;
     }
 
+    private boolean notEquals(ServiceUnitStateData from, ServiceUnitStateData to) {
+        return !from.broker().equals(to.broker())
+                || !StringUtils.equals(from.sourceBroker(), to.sourceBroker());
+    }
+
+    private boolean invalidTransfer(ServiceUnitStateData from, ServiceUnitStateData to) {
+        return !from.broker().equals(to.sourceBroker())

Review Comment:
   why aren't we using StringUtils.equals for every comparison ?



-- 
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: commits-unsubscribe@pulsar.apache.org

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