Charles's comments

.gitignore

@@ -1,3 +1,5 @@
+# *.xml line was added by cbrennan
+*.xml
 *.swp
 *.pyc
 be/output

fe/fe-core/src/main/java/com/starrocks/common/util/PropertyAnalyzer.java

@@ -150,6 +150,8 @@ public class PropertyAnalyzer {
 
     public static final String PROPERTIES_LABELS_LOCATION = "labels.location";
     public static final String PROPERTIES_LABELS_GROUP = "labels.group";
+    private static final String COLON_DELIMITER = ":";
+    private static final String PROPERTIES_GROUP = "group";
 
     public static final String PROPERTIES_PERSISTENT_INDEX_TYPE = "persistent_index_type";
 
@@ -949,8 +951,8 @@ public static String convertLocationMapToString(Multimap<String, String> locatio
     public static String getResourceIsolationGroupFromProperties(Map<String, String> properties)
             throws UnsupportedOperationException {
         String entry = properties.get(PROPERTIES_LABELS_GROUP);
-        String[] groupKV = entry.split(":");
-        if (groupKV.length != 2 || !groupKV[0].trim().equals("group")) {
+        String[] groupKV = entry.split(COLON_DELIMITER);
+        if (groupKV.length != 2 || !groupKV[0].trim().equals(PROPERTIES_GROUP)) {
             throw new UnsupportedOperationException("the group property must be formatted 'group:<GROUP_ID>'");
         }
         return groupKV[1].trim();

fe/fe-core/src/main/java/com/starrocks/server/NodeMgr.java

@@ -785,6 +785,10 @@ public void modifyFrontend(ModifyFrontendClause modifyFrontendClause) throws Ddl
         if (modifyFrontendClause.getSrcHost() != null || modifyFrontendClause.getDestHost() != null) {
             throw new DdlException("Malformed ModifyFrontendClause. Shouldn't set properties and change host address.");
         }
+        Map<String, String> props = modifyFrontendClause.getProperties();
+        if (!props.containsKey(PROPERTIES_LABELS_GROUP)) {
+            throw new DdlException("Setting non-'group' property on frontend is not supported.");
+        }
         if (!tryLock(false)) {
             throw new DdlException("Failed to acquire globalStateMgr lock. Try again");
         }
@@ -799,10 +803,6 @@ public void modifyFrontend(ModifyFrontendClause modifyFrontendClause) throws Ddl
             // note that we don't need to update the info in BDB since it's not relevant to BDB to which group the
             // frontend belongs.
             // step 1 update the fe information stored in memory.
-            Map<String, String> props = modifyFrontendClause.getProperties();
-            if (!props.containsKey(PROPERTIES_LABELS_GROUP)) {
-                throw new DdlException("Setting non-'group' property on frontend is not supported.");
-            }
             fe.setResourceIsolationGroup(getResourceIsolationGroupFromProperties(props));
             // step 2 editLog
             GlobalStateMgr.getCurrentState().getEditLog().logUpdateFrontend(fe);

fe/fe-core/src/main/java/com/starrocks/system/SystemInfoService.java

@@ -302,7 +302,7 @@ public ShowResultSet modifyComputeNodeProperty(ModifyComputeNodeClause modifyCom
         // check compute node existence
         ComputeNode computeNode = getComputeNodeWithHeartbeatPort(computeNodeHostPort.split(":")[0],
                 Integer.parseInt(computeNodeHostPort.split(":")[1]));
-        if (null == computeNode) {
+        if (computeNode == null) {
             throw new DdlException(String.format("computeNode [%s] not found", computeNodeHostPort));
         }
 
@@ -318,9 +318,12 @@ public ShowResultSet modifyComputeNodeProperty(ModifyComputeNodeClause modifyCom
                     computeNode.setResourceIsolationGroup("");
                     continue;
                 }
-                computeNode.setResourceIsolationGroup(getResourceIsolationGroupFromProperties(properties));
-                String opMessage = String.format("%s:%d's group has been modified to %s",
-                        computeNode.getHost(), computeNode.getHeartbeatPort(), properties);
+                String oldResourceIsolationGroup = computeNode.getResourceIsolationGroup();
+                String newResourceIsolationGroup = getResourceIsolationGroupFromProperties(properties);
+                computeNode.setResourceIsolationGroup(newResourceIsolationGroup);
+                String opMessage = String.format("%s:%d's group has been modified from %s to %s",
+                        computeNode.getHost(), computeNode.getHeartbeatPort(),
+                        oldResourceIsolationGroup, newResourceIsolationGroup);
                 messageResult.add(Collections.singletonList(opMessage));
             } else {
                 throw new UnsupportedOperationException("unsupported property: " + entry.getKey());
@@ -1152,6 +1155,8 @@ public void updateInMemoryStateBackend(Backend persistentState) {
             // 1. SystemHandler drop the decommission backend
             // 2. at same time, user try to cancel the decommission of that backend.
             // These two operations do not guarantee the order.
+            LOG.warn(String.format("Not updating in memory state for backend, could be legitimate reason or bug: [%s]",
+                    persistentState));
             return;
         }
         updateCommonInMemoryState(persistentState, inMemoryState);
@@ -1167,6 +1172,8 @@ public void updateInMemoryStateComputeNode(ComputeNode persistentState) {
             // 1. SystemHandler drop the decommission compute node
             // 2. at same time, user try to cancel the decommission of that compute node.
             // These two operations do not guarantee the order.
+            LOG.warn(String.format("Not updating in memory state for compute node, could be legitimate reason or bug:" +
+                    " [%s]", persistentState));
             return;
         }
         updateCommonInMemoryState(persistentState, inMemoryState);

fe/fe-core/src/test/java/com/starrocks/system/SystemInfoServiceTest.java

@@ -139,7 +139,6 @@ public void testModifyBackendGroupPropertyThrowsException() throws DdlException
         Backend be = new Backend(100, "originalHost", 1000);
         service.addBackend(be);
         Map<String, String> properties = Maps.newHashMap();
-        String location = "rack:rack1";
         properties.put(AlterSystemStmtAnalyzer.PROP_KEY_GROUP, "group:writegroup");
         ModifyBackendClause clause = new ModifyBackendClause("originalHost:1000", properties);
         service.modifyBackendProperty(clause);
@@ -158,6 +157,28 @@ public void testModifyComputeNodeGroupProperty() throws DdlException {
         Assert.assertEquals("writegroup", computeNode.getResourceIsolationGroup());
     }
 
+    @Test(expected = UnsupportedOperationException.class)
+    public void testModifyComputeNodeBadGroupProperty1ThrowsException() throws DdlException {
+        ComputeNode cn = new ComputeNode(100, "originalHost", 1000);
+        service.addComputeNode(cn);
+        Map<String, String> properties = Maps.newHashMap();
+        // missing the "group:" prefix
+        properties.put(AlterSystemStmtAnalyzer.PROP_KEY_GROUP, "writegroup");
+        ModifyComputeNodeClause clause = new ModifyComputeNodeClause("originalHost:1000", properties);
+        service.modifyComputeNodeProperty(clause);
+    }
+
+    @Test(expected = UnsupportedOperationException.class)
+    public void testModifyComputeNodeBadGroupProperty2ThrowsException() throws DdlException {
+        ComputeNode cn = new ComputeNode(100, "originalHost", 1000);
+        service.addComputeNode(cn);
+        Map<String, String> properties = Maps.newHashMap();
+        // bad spaces
+        properties.put(AlterSystemStmtAnalyzer.PROP_KEY_GROUP, " group : writegroup ");
+        ModifyComputeNodeClause clause = new ModifyComputeNodeClause("originalHost:1000", properties);
+        service.modifyComputeNodeProperty(clause);
+    }
+
     @Test
     public void testUpdateBackend() throws Exception {
         Backend be = new Backend(10001, "newHost", 1000);