This is an automated email from the ASF dual-hosted git repository.
domgarguilo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/main by this push:
new 9a5d282746 ResourceGroupId validation/usage improvements (#5893)
9a5d282746 is described below
commit 9a5d282746aeb10180ea51b5c5e686e6e63bdced
Author: Dom G. <[email protected]>
AuthorDate: Mon Sep 22 10:00:55 2025 -0400
ResourceGroupId validation/usage improvements (#5893)
* removed unneeded public API from ResourceGroupId
* improved group ID validation methods and exception message
* improved the usage of ResourceGroupId collections in ClusterConfigParser
* consolidated ResourceGroupId validation test cases
---
.../apache/accumulo/core/data/ResourceGroupId.java | 17 ++++--
.../accumulo/core/data/ResourceGroupIdTest.java | 24 +++++++--
.../server/conf/cluster/ClusterConfigParser.java | 50 ++++++++----------
.../conf/cluster/ClusterConfigParserTest.java | 60 +++++++---------------
4 files changed, 72 insertions(+), 79 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/data/ResourceGroupId.java
b/core/src/main/java/org/apache/accumulo/core/data/ResourceGroupId.java
index 151a26094a..e2954514fa 100644
--- a/core/src/main/java/org/apache/accumulo/core/data/ResourceGroupId.java
+++ b/core/src/main/java/org/apache/accumulo/core/data/ResourceGroupId.java
@@ -28,7 +28,7 @@ import com.github.benmanes.caffeine.cache.Cache;
public class ResourceGroupId extends AbstractId<ResourceGroupId> {
- public static final Pattern GROUP_NAME_PATTERN =
Pattern.compile("^[a-zA-Z]+(_?[a-zA-Z0-9])*$");
+ private static final Pattern GROUP_NAME_PATTERN =
Pattern.compile("^[a-zA-Z]+(_?[a-zA-Z0-9])*$");
// cache is for canonicalization/deduplication of created objects,
// to limit the number of ResourceGroupId objects in the JVM at any given
moment
@@ -43,16 +43,23 @@ public class ResourceGroupId extends
AbstractId<ResourceGroupId> {
private ResourceGroupId(String canonical) {
super(canonical);
- if (!GROUP_NAME_PATTERN.matcher(canonical).matches()) {
- throw new IllegalArgumentException(
- "Group name: " + canonical + " contains invalid characters");
+ validateGroupName(canonical);
+ }
+
+ /**
+ * @throws IllegalArgumentException if the group name is invalid
+ */
+ private static void validateGroupName(String groupName) {
+ if (!GROUP_NAME_PATTERN.matcher(groupName).matches()) {
+ throw new IllegalArgumentException("Group name '" + groupName
+ + "' is invalid. Valid names must match the pattern: " +
GROUP_NAME_PATTERN.pattern());
}
}
/**
* Get a ResourceGroupId object for the provided canonical string.
*
- * @param canonical table ID string
+ * @param canonical resource group ID string
* @return ResourceGroupId object
*/
public static ResourceGroupId of(final String canonical) {
diff --git
a/core/src/test/java/org/apache/accumulo/core/data/ResourceGroupIdTest.java
b/core/src/test/java/org/apache/accumulo/core/data/ResourceGroupIdTest.java
index 9906ac2684..3128ac798e 100644
--- a/core/src/test/java/org/apache/accumulo/core/data/ResourceGroupIdTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/data/ResourceGroupIdTest.java
@@ -23,20 +23,38 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
import org.junit.jupiter.api.Test;
public class ResourceGroupIdTest {
+
+ @Test
+ public void testValidIds() {
+ ResourceGroupId.of("a");
+ ResourceGroupId.of("b");
+ ResourceGroupId.of("default");
+ ResourceGroupId.of("reg_ular");
+ ResourceGroupId.of("group1");
+ ResourceGroupId.of("validGroup");
+ }
+
@Test
public void testIllegalIds() {
assertThrows(IllegalArgumentException.class, () -> ResourceGroupId.of(""));
assertThrows(IllegalArgumentException.class, () -> ResourceGroupId.of("
"));
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("\t"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("group1 "));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("group 1"));
+
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("_"));
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("9"));
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("9group1"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("invalid_Group_"));
+
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("$"));
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("$group1"));
- assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("group1 "));
- assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("group 1"));
assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("gro$up1"));
- assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("invalid_Group_"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("a-b"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("a*b"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("a?b"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("group@name"));
+ assertThrows(IllegalArgumentException.class, () ->
ResourceGroupId.of("group#name"));
}
}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParser.java
b/server/base/src/main/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParser.java
index 4666668e9b..916524ed9f 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParser.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParser.java
@@ -26,10 +26,10 @@ import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.HashMap;
-import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Predicate;
import java.util.stream.Collectors;
@@ -52,14 +52,6 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
justification = "visible for testing")
public class ClusterConfigParser {
- static void validateGroupNames(List<String> names) {
- for (String name : names) {
- if (!ResourceGroupId.GROUP_NAME_PATTERN.matcher(name).matches()) {
- throw new IllegalArgumentException("Group name: " + name + " contains
invalid characters");
- }
- }
- }
-
// visible for testing
public static SiteConfiguration siteConf = null;
@@ -129,10 +121,10 @@ public class ClusterConfigParser {
}
}
- private static List<String> parseGroup(Map<String,String> config, String
prefix) {
+ private static TreeSet<ResourceGroupId> parseGroup(Map<String,String>
config, String prefix) {
Preconditions.checkArgument(prefix.equals(COMPACTOR_PREFIX) ||
prefix.equals(SSERVER_PREFIX)
|| prefix.equals(TSERVER_PREFIX));
- List<String> groups = config.keySet().stream().filter(k ->
k.startsWith(prefix)).map(k -> {
+ return config.keySet().stream().filter(k -> k.startsWith(prefix)).map(k ->
{
int periods = StringUtils.countMatches(k, '.');
if (periods == 1) {
return k.substring(prefix.length());
@@ -148,18 +140,17 @@ public class ClusterConfigParser {
} else {
throw new IllegalArgumentException("Malformed configuration, has too
many levels: " + k);
}
- }).sorted().distinct().collect(Collectors.toList());
- validateGroupNames(groups);
- return groups;
+ }).map(ResourceGroupId::of).collect(Collectors.toCollection(TreeSet::new));
}
- private static void validateConfiguredGroups(final ServerContext ctx, final
Set<String> zkGroups,
- final List<String> configuredGroups, boolean createMissingRG) {
- for (String cg : configuredGroups) {
+ private static void validateConfiguredGroups(final ServerContext ctx,
+ final Set<ResourceGroupId> zkGroups, final Set<ResourceGroupId>
configuredGroups,
+ boolean createMissingRG) {
+ for (ResourceGroupId cg : configuredGroups) {
if (!zkGroups.contains(cg)) {
if (createMissingRG) {
try {
- ctx.resourceGroupOperations().create(ResourceGroupId.of(cg));
+ ctx.resourceGroupOperations().create(cg);
} catch (AccumuloException | AccumuloSecurityException e) {
throw new IllegalStateException("Error creating resource group: "
+ cg, e);
}
@@ -173,7 +164,7 @@ public class ClusterConfigParser {
}
public static void outputShellVariables(ServerContext ctx,
Map<String,String> config,
- Set<String> zkGroups, boolean createMissingRG, PrintStream out) {
+ Set<ResourceGroupId> zkGroups, boolean createMissingRG, PrintStream out)
{
// find invalid config sections and point the user to the first one
config.keySet().stream().filter(VALID_CONFIG_SECTIONS.negate()).findFirst()
@@ -192,7 +183,7 @@ public class ClusterConfigParser {
}
}
- List<String> compactorGroups = parseGroup(config, COMPACTOR_PREFIX);
+ var compactorGroups = parseGroup(config, COMPACTOR_PREFIX);
if (compactorGroups.isEmpty()) {
throw new IllegalArgumentException(
"No compactor groups found, at least one compactor group is required
to compact the system tables.");
@@ -200,8 +191,8 @@ public class ClusterConfigParser {
validateConfiguredGroups(ctx, zkGroups, compactorGroups, createMissingRG);
if (!compactorGroups.isEmpty()) {
out.printf(PROPERTY_FORMAT, "COMPACTOR_GROUPS",
- compactorGroups.stream().collect(Collectors.joining(" ")));
- for (String group : compactorGroups) {
+ String.join(" ",
compactorGroups.stream().map(ResourceGroupId::canonical).toList()));
+ for (ResourceGroupId group : compactorGroups) {
out.printf(PROPERTY_FORMAT, "COMPACTOR_HOSTS_" + group,
config.get(COMPACTOR_PREFIX + group + HOSTS_SUFFIX));
String numCompactors =
@@ -210,18 +201,18 @@ public class ClusterConfigParser {
}
}
- List<String> sserverGroups = parseGroup(config, SSERVER_PREFIX);
+ var sserverGroups = parseGroup(config, SSERVER_PREFIX);
validateConfiguredGroups(ctx, zkGroups, sserverGroups, createMissingRG);
if (!sserverGroups.isEmpty()) {
out.printf(PROPERTY_FORMAT, "SSERVER_GROUPS",
- sserverGroups.stream().collect(Collectors.joining(" ")));
+ String.join(" ",
sserverGroups.stream().map(ResourceGroupId::canonical).toList()));
sserverGroups.forEach(ssg -> out.printf(PROPERTY_FORMAT,
"SSERVER_HOSTS_" + ssg,
config.get(SSERVER_PREFIX + ssg + HOSTS_SUFFIX)));
sserverGroups.forEach(ssg -> out.printf(PROPERTY_FORMAT,
"SSERVERS_PER_HOST_" + ssg,
config.getOrDefault(SSERVER_PREFIX + ssg + SERVERS_PER_HOST_SUFFIX,
"1")));
}
- List<String> tserverGroups = parseGroup(config, TSERVER_PREFIX);
+ var tserverGroups = parseGroup(config, TSERVER_PREFIX);
if (tserverGroups.isEmpty()) {
throw new IllegalArgumentException(
"No tserver groups found, at least one tserver group is required to
host the system tables.");
@@ -230,7 +221,7 @@ public class ClusterConfigParser {
AtomicBoolean foundTServer = new AtomicBoolean(false);
if (!tserverGroups.isEmpty()) {
out.printf(PROPERTY_FORMAT, "TSERVER_GROUPS",
- tserverGroups.stream().collect(Collectors.joining(" ")));
+ String.join(" ",
tserverGroups.stream().map(ResourceGroupId::canonical).toList()));
tserverGroups.forEach(tsg -> {
String hosts = config.get(TSERVER_PREFIX + tsg + HOSTS_SUFFIX);
foundTServer.compareAndSet(false, hosts != null && !hosts.isEmpty());
@@ -264,12 +255,11 @@ public class ClusterConfigParser {
if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
SecurityUtil.serverLogin(siteConf);
}
- final Set<String> zkGroups = new HashSet<>();
- context.resourceGroupOperations().list().forEach(rg ->
zkGroups.add(rg.canonical()));
- if (!zkGroups.contains(ResourceGroupId.DEFAULT.canonical())) {
+ final Set<ResourceGroupId> zkGroups = new
TreeSet<>(context.resourceGroupOperations().list());
+ if (!zkGroups.contains(ResourceGroupId.DEFAULT)) {
throw new IllegalStateException("Default resource group not found in
ZooKeeper");
}
- boolean createMissingRG = args[0].equals("true") ? true : false;
+ boolean createMissingRG = args[0].equals("true");
try {
if (args.length == 3) {
// Write to a file instead of System.out if provided as an argument
diff --git
a/server/base/src/test/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParserTest.java
b/server/base/src/test/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParserTest.java
index 9d397e8173..e5f0e7a0c9 100644
---
a/server/base/src/test/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParserTest.java
+++
b/server/base/src/test/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParserTest.java
@@ -30,11 +30,12 @@ import java.io.PrintStream;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.List;
+import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.function.Function;
+import java.util.stream.Stream;
import org.apache.accumulo.core.data.ResourceGroupId;
import org.apache.accumulo.server.WithTestNames;
@@ -116,11 +117,11 @@ public class ClusterConfigParserTest extends
WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
final Path outputFile = Files.createFile(tempDir.resolve(testName()));
-
+ var groups = new HashSet<ResourceGroupId>();
+ groups.add(ResourceGroupId.DEFAULT);
+ Stream.of("q1", "q2", "cheap",
"highmem").map(ResourceGroupId::of).forEach(groups::add);
try (var out = Files.newOutputStream(outputFile); var ps = new
PrintStream(out)) {
- ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical(), "q1", "q2", "cheap",
"highmem"), false,
- ps);
+ ClusterConfigParser.outputShellVariables(null, contents, groups,
false, ps);
}
return outputFile;
@@ -183,9 +184,8 @@ public class ClusterConfigParserTest extends WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
- var exception = assertThrows(IllegalArgumentException.class,
- () -> ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical()), false, ps));
+ var exception = assertThrows(IllegalArgumentException.class, () ->
ClusterConfigParser
+ .outputShellVariables(null, contents,
Set.of(ResourceGroupId.DEFAULT), false, ps));
assertTrue(exception.getMessage().contains("vserver"));
}
}
@@ -200,9 +200,8 @@ public class ClusterConfigParserTest extends WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
- var exception =
- assertThrows(RuntimeException.class, () ->
ClusterConfigParser.outputShellVariables(null,
- contents, Set.of(ResourceGroupId.DEFAULT.canonical()), false,
ps));
+ var exception = assertThrows(RuntimeException.class, () ->
ClusterConfigParser
+ .outputShellVariables(null, contents,
Set.of(ResourceGroupId.DEFAULT), false, ps));
assertTrue(exception.getMessage().contains("bad-group-name"));
}
}
@@ -217,9 +216,8 @@ public class ClusterConfigParserTest extends WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
- var exception = assertThrows(IllegalArgumentException.class,
- () -> ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical()), false, ps));
+ var exception = assertThrows(IllegalArgumentException.class, () ->
ClusterConfigParser
+ .outputShellVariables(null, contents,
Set.of(ResourceGroupId.DEFAULT), false, ps));
assertTrue(exception.getMessage().contains("servers_per_hosts"));
}
}
@@ -234,9 +232,8 @@ public class ClusterConfigParserTest extends WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
- var exception = assertThrows(IllegalArgumentException.class,
- () -> ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical()), false, ps));
+ var exception = assertThrows(IllegalArgumentException.class, () ->
ClusterConfigParser
+ .outputShellVariables(null, contents,
Set.of(ResourceGroupId.DEFAULT), false, ps));
assertTrue(exception.getMessage().contains("too many levels"));
}
}
@@ -251,9 +248,8 @@ public class ClusterConfigParserTest extends WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
- var exception = assertThrows(IllegalArgumentException.class,
- () -> ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical()), false, ps));
+ var exception = assertThrows(IllegalArgumentException.class, () ->
ClusterConfigParser
+ .outputShellVariables(null, contents,
Set.of(ResourceGroupId.DEFAULT), false, ps));
assertTrue(exception.getMessage().contains("No compactor groups found"));
}
}
@@ -270,7 +266,7 @@ public class ClusterConfigParserTest extends WithTestNames {
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
var exception = assertThrows(IllegalArgumentException.class,
() -> ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical(), "q1"), false, ps));
+ Set.of(ResourceGroupId.DEFAULT, ResourceGroupId.of("q1")),
false, ps));
assertTrue(exception.getMessage().contains("No tserver groups found"));
}
}
@@ -285,28 +281,10 @@ public class ClusterConfigParserTest extends
WithTestNames {
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
try (var baos = new ByteArrayOutputStream(); var ps = new
PrintStream(baos)) {
- var exception = assertThrows(IllegalArgumentException.class,
- () -> ClusterConfigParser.outputShellVariables(null, contents,
- Set.of(ResourceGroupId.DEFAULT.canonical()), false, ps));
+ var exception = assertThrows(IllegalArgumentException.class, () ->
ClusterConfigParser
+ .outputShellVariables(null, contents,
Set.of(ResourceGroupId.DEFAULT), false, ps));
assertTrue(exception.getMessage().contains("Check the format"));
}
}
- @Test
- public void testGroupNamePattern() {
- ClusterConfigParser.validateGroupNames(List.of("a"));
- ClusterConfigParser.validateGroupNames(List.of("a", "b"));
- ClusterConfigParser.validateGroupNames(List.of("default", "reg_ular"));
- assertThrows(RuntimeException.class,
- () -> ClusterConfigParser.validateGroupNames(List.of("a1b2c3d4__")));
- assertThrows(RuntimeException.class,
- () -> ClusterConfigParser.validateGroupNames(List.of("0abcde")));
- assertThrows(RuntimeException.class,
- () -> ClusterConfigParser.validateGroupNames(List.of("a-b")));
- assertThrows(RuntimeException.class,
- () -> ClusterConfigParser.validateGroupNames(List.of("a*b")));
- assertThrows(RuntimeException.class,
- () -> ClusterConfigParser.validateGroupNames(List.of("a?b")));
- }
-
}
