diff --git a/helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java b/helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java index f0002bdbde..03669259f3 100644 --- a/helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java +++ b/helix-core/src/main/java/org/apache/helix/PropertyPathBuilder.java @@ -19,8 +19,10 @@ * under the License. */ +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -49,8 +51,8 @@ public class PropertyPathBuilder { private static Logger logger = LoggerFactory.getLogger(PropertyPathBuilder.class); - static final Map> templateMap = - new HashMap>(); + static final Map> templateMap = + new HashMap>(); @Deprecated // typeToClassMapping is not being used anywhere static final Map> typeToClassMapping = new HashMap>(); @@ -180,15 +182,67 @@ public class PropertyPathBuilder { addEntry(PropertyType.JOB_CONTEXT, 3, "/{clusterName}/PROPERTYSTORE" + TaskConstants.REBALANCER_CONTEXT_ROOT + "/{workflowName}" + "_" + "{jobName}/Context"); } - static Pattern pattern = Pattern.compile("(\\{.+?\\})"); + + /** + * Pre-parsed path template for efficient path construction. + * Parses template strings once at initialization to avoid repeated regex operations. + */ + static class PathTemplate { + private final String[] parts; + + /** + * Parse a template string like "/{clusterName}/INSTANCES/{instanceName}" + * into reusable parts: ["/", "/INSTANCES/", ""] + */ + PathTemplate(String template) { + List partsList = new ArrayList<>(); + int lastEnd = 0; + + // Find all {placeholders} and split the template into static parts + int start = template.indexOf('{'); + while (start != -1) { + partsList.add(template.substring(lastEnd, start)); + int end = template.indexOf('}', start); + if (end == -1) { + logger.error("Malformed template: missing closing brace in " + template); + break; + } + lastEnd = end + 1; + start = template.indexOf('{', lastEnd); + } + partsList.add(template.substring(lastEnd)); + + this.parts = partsList.toArray(new String[0]); + } + + /** + * Build path by concatenating static parts with parameters. + */ + String buildPath(String clusterName, String... keys) { + StringBuilder sb = new StringBuilder(128); + sb.append(parts[0]).append(clusterName); + + for (int i = 0; i < keys.length; i++) { + if (i + 1 < parts.length) { + sb.append(parts[i + 1]); + } + sb.append(keys[i]); + } + + if (keys.length + 1 < parts.length) { + sb.append(parts[keys.length + 1]); + } + + return sb.toString(); + } + } private static void addEntry(PropertyType type, int numKeys, String template) { if (!templateMap.containsKey(type)) { - templateMap.put(type, new HashMap()); + templateMap.put(type, new HashMap()); } - logger.trace("Adding template for type:" + type.getType() + " arguments:" + numKeys - + " template:" + template); - templateMap.get(type).put(numKeys, template); + logger.trace("Adding template for type:" + type.getType() + " arguments:" + numKeys + " template:" + template); + templateMap.get(type).put(numKeys, new PathTemplate(template)); } /** @@ -204,33 +258,22 @@ public static String getPath(PropertyType type, String clusterName, String... ke return null; } if (keys == null) { - keys = new String[] {}; + keys = new String[]{}; } - String template = null; + + PathTemplate template = null; if (templateMap.containsKey(type)) { - // keys.length+1 since we add clusterName template = templateMap.get(type).get(keys.length + 1); } String result = null; - if (template != null) { - result = template; - Matcher matcher = pattern.matcher(template); - int count = 0; - while (matcher.find()) { - count = count + 1; - String var = matcher.group(); - if (count == 1) { - result = result.replace(var, clusterName); - } else { - result = result.replace(var, keys[count - 2]); - } - } + result = template.buildPath(clusterName, keys); } + if (result == null || result.indexOf('{') > -1 || result.indexOf('}') > -1) { - logger.warn("Unable to instantiate template:" + template + " using clusterName:" + clusterName - + " and keys:" + Arrays.toString(keys)); + logger.warn("Unable to instantiate template for type:" + type + " using clusterName:" + clusterName + " and keys:" + + Arrays.toString(keys)); } return result; } diff --git a/helix-core/src/test/java/org/apache/helix/TestPropertyPathBuilder.java b/helix-core/src/test/java/org/apache/helix/TestPropertyPathBuilder.java index 9212568356..6d3b3fbe0d 100644 --- a/helix-core/src/test/java/org/apache/helix/TestPropertyPathBuilder.java +++ b/helix-core/src/test/java/org/apache/helix/TestPropertyPathBuilder.java @@ -42,10 +42,8 @@ public void testGetPath() { actual = PropertyPathBuilder.instanceTaskCurrentState("test_cluster", "instanceName1"); AssertJUnit.assertEquals(actual, "/test_cluster/INSTANCES/instanceName1/TASKCURRENTSTATES"); - actual = - PropertyPathBuilder.instanceTaskCurrentState("test_cluster", "instanceName1", "sessionId"); - AssertJUnit - .assertEquals(actual, "/test_cluster/INSTANCES/instanceName1/TASKCURRENTSTATES/sessionId"); + actual = PropertyPathBuilder.instanceTaskCurrentState("test_cluster", "instanceName1", "sessionId"); + AssertJUnit.assertEquals(actual, "/test_cluster/INSTANCES/instanceName1/TASKCURRENTSTATES/sessionId"); actual = PropertyPathBuilder.instanceCustomizedState("test_cluster", "instanceName1"); AssertJUnit.assertEquals(actual, "/test_cluster/INSTANCES/instanceName1/CUSTOMIZEDSTATES"); @@ -60,4 +58,80 @@ public void testGetPath() { actual = PropertyPathBuilder.clusterStatus("test_cluster"); Assert.assertEquals(actual, "/test_cluster/STATUS/CLUSTER/test_cluster"); } + + /** + * Test the core getPath() method with the optimized PathTemplate implementation. + * Tests paths with varying numbers of parameters (0-4 keys). + */ + @Test + public void testGetPathWithPropertyType() { + String actual; + + actual = + PropertyPathBuilder.getPath(PropertyType.CURRENTSTATES, "MyCluster", "instance1", "session123", "resource1"); + Assert.assertEquals(actual, "/MyCluster/INSTANCES/instance1/CURRENTSTATES/session123/resource1"); + + actual = + PropertyPathBuilder.getPath(PropertyType.CURRENTSTATES, "MyCluster", "instance1", "session123", "resource1", + "bucket1"); + Assert.assertEquals(actual, "/MyCluster/INSTANCES/instance1/CURRENTSTATES/session123/resource1/bucket1"); + + actual = + PropertyPathBuilder.getPath(PropertyType.CUSTOMIZEDSTATES, "MyCluster", "instance1", "MyState", "MyResource"); + Assert.assertEquals(actual, "/MyCluster/INSTANCES/instance1/CUSTOMIZEDSTATES/MyState/MyResource"); + + actual = PropertyPathBuilder.getPath(PropertyType.CUSTOMIZEDVIEW, "MyCluster"); + Assert.assertEquals(actual, "/MyCluster/CUSTOMIZEDVIEW"); + + actual = PropertyPathBuilder.getPath(PropertyType.CUSTOMIZEDVIEW, "MyCluster", "customType"); + Assert.assertEquals(actual, "/MyCluster/CUSTOMIZEDVIEW/customType"); + + actual = PropertyPathBuilder.getPath(PropertyType.CUSTOMIZEDVIEW, "MyCluster", "customType", "resource1"); + Assert.assertEquals(actual, "/MyCluster/CUSTOMIZEDVIEW/customType/resource1"); + + // Test that getPath() works correctly for basic cases + actual = PropertyPathBuilder.getPath(PropertyType.LIVEINSTANCES, "MyCluster", "node1"); + Assert.assertEquals(actual, "/MyCluster/LIVEINSTANCES/node1"); + + actual = PropertyPathBuilder.getPath(PropertyType.IDEALSTATES, "MyCluster", "MyResource"); + Assert.assertEquals(actual, "/MyCluster/IDEALSTATES/MyResource"); + + actual = + PropertyPathBuilder.getPath(PropertyType.STATUSUPDATES, "MyCluster", "inst1", "sess1", "subpath", "record1"); + Assert.assertEquals(actual, "/MyCluster/INSTANCES/inst1/STATUSUPDATES/sess1/subpath/record1"); + + actual = PropertyPathBuilder.getPath(PropertyType.ERRORS, "MyCluster", "inst1", "sess1", "subpath", "record1"); + Assert.assertEquals(actual, "/MyCluster/INSTANCES/inst1/ERRORS/sess1/subpath/record1"); + + actual = PropertyPathBuilder.getPath(PropertyType.TASKCURRENTSTATES, "MyCluster", "inst1", "sess1", "resource1", + "bucket1"); + Assert.assertEquals(actual, "/MyCluster/INSTANCES/inst1/TASKCURRENTSTATES/sess1/resource1/bucket1"); + + actual = PropertyPathBuilder.getPath(PropertyType.CONFIGS, "MyCluster", "PARTICIPANT", "instance1"); + Assert.assertEquals(actual, "/MyCluster/CONFIGS/PARTICIPANT/instance1"); + } + + /** + * Test edge cases and special scenarios + */ + @Test + public void testGetPathEdgeCases() { + String actual; + + // Test with null clusterName - should return null and log warning + actual = PropertyPathBuilder.getPath(PropertyType.IDEALSTATES, null); + Assert.assertNull(actual); + + // Test with null keys array - should work (treated as empty array) + actual = PropertyPathBuilder.getPath(PropertyType.LIVEINSTANCES, "TestCluster", (String[]) null); + Assert.assertEquals(actual, "/TestCluster/LIVEINSTANCES"); + + // Test with cluster name containing special characters + actual = PropertyPathBuilder.getPath(PropertyType.IDEALSTATES, "test-cluster_123"); + Assert.assertEquals(actual, "/test-cluster_123/IDEALSTATES"); + + // Test with resource name containing special characters + actual = PropertyPathBuilder.getPath(PropertyType.IDEALSTATES, "cluster", "resource-name_123.test"); + Assert.assertEquals(actual, "/cluster/IDEALSTATES/resource-name_123.test"); + } }