From 2029aecffd53f9b2b614fc5adc0f12f8f73ba947 Mon Sep 17 00:00:00 2001 From: Connor Date: Wed, 4 Sep 2024 15:20:16 -0400 Subject: [PATCH] Charles's comments --- .gitignore | 2 ++ .../common/util/PropertyAnalyzer.java | 6 +++-- .../java/com/starrocks/server/NodeMgr.java | 8 +++---- .../starrocks/system/SystemInfoService.java | 15 ++++++++---- .../system/SystemInfoServiceTest.java | 23 ++++++++++++++++++- 5 files changed, 43 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index 749a2ce168fa4..cc57cb43c60bd 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,5 @@ +# *.xml line was added by cbrennan +*.xml *.swp *.pyc be/output diff --git a/fe/fe-core/src/main/java/com/starrocks/common/util/PropertyAnalyzer.java b/fe/fe-core/src/main/java/com/starrocks/common/util/PropertyAnalyzer.java index 939705bf4e35b..91c062cb6011b 100644 --- a/fe/fe-core/src/main/java/com/starrocks/common/util/PropertyAnalyzer.java +++ b/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 locatio public static String getResourceIsolationGroupFromProperties(Map 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:'"); } return groupKV[1].trim(); diff --git a/fe/fe-core/src/main/java/com/starrocks/server/NodeMgr.java b/fe/fe-core/src/main/java/com/starrocks/server/NodeMgr.java index e5b3879d235a4..aabaa9483aec8 100644 --- a/fe/fe-core/src/main/java/com/starrocks/server/NodeMgr.java +++ b/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 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 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); diff --git a/fe/fe-core/src/main/java/com/starrocks/system/SystemInfoService.java b/fe/fe-core/src/main/java/com/starrocks/system/SystemInfoService.java index 8fae01a9fa1e7..796c89363bf45 100644 --- a/fe/fe-core/src/main/java/com/starrocks/system/SystemInfoService.java +++ b/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); diff --git a/fe/fe-core/src/test/java/com/starrocks/system/SystemInfoServiceTest.java b/fe/fe-core/src/test/java/com/starrocks/system/SystemInfoServiceTest.java index 931108b021414..f9f87532b4310 100644 --- a/fe/fe-core/src/test/java/com/starrocks/system/SystemInfoServiceTest.java +++ b/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 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 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 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);