Skip to content

Improve Property Types #6487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Amemeda wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Improve Property Types #6487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a702db0
Move required properties out of ServerConfigCheckRunner
Amemeda Jul 21, 2026
a44536b
Fix incorrect return type in comment
Amemeda Jul 21, 2026
f9d82be
Add validation for PropertyType.URI and PropertyType.PATH
Amemeda Jul 22, 2026
a77bedb
Fix failing unit test for PropertyType.PATH
Amemeda Jul 22, 2026
b993313
Fix checkstyle violdations
Amemeda Jul 23, 2026
c499e66
Remove outdated TODO from PropertyType.java, apache#2699
Amemeda Jul 23, 2026
335922d
Simplify matches regex for PropertyType.PATH validation
Amemeda Jul 23, 2026
d472f41
Update Javadoc for required properties and validUri(), removed the se…
Amemeda Jul 31, 2026
ec0c0ae
Remove unneccessary ValidPath method
Amemeda Aug 4, 2026
2ed6245
Switch out method to return required properties for a final enum set …
Amemeda Aug 4, 2026
cb9f762
Add field isRequired to enum Property, remove set of required properties
Amemeda Aug 7, 2026
fe164c9
Add method to Property to validate required property values
Amemeda Aug 13, 2026
661d99b
Fix checkstyle violation
Amemeda Aug 13, 2026
d51430c
Change required properties to have null default value
Amemeda Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions core/src/main/java/org/apache/accumulo/core/conf/ConfigurationDocGen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,19 @@ void property(Property prop) {
description += strike(sanitize(prop.getDescription()), depr) + "<br>"
+ strike("**type:** " + prop.getType().name(), depr) + ", "
+ strike("**zk mutable:** " + isZooKeeperMutable(prop), depr) + ", ";
String defaultValue = sanitize(prop.getDefaultValue()).trim();
if (defaultValue.isEmpty()) {
description += strike("**default value:** empty", depr);
} else if (defaultValue.contains("\n")) {
// deal with multi-line values, skip strikethrough of value
description += strike("**default value:** ", depr) + "\n```\n" + defaultValue + "\n```\n";
} else if (prop.getType() == PropertyType.CLASSNAME
&& defaultValue.startsWith("org.apache.accumulo")) {
description += strike("**default value:** {% jlink -f " + defaultValue + " %}", depr);
} else {
description += strike("**default value:** `" + defaultValue + "`", depr);
if (prop.getDefaultValue() != null) {
String defaultValue = sanitize(prop.getDefaultValue()).trim();
if (defaultValue.isEmpty()) {
description += strike("**default value:** empty", depr);
} else if (defaultValue.contains("\n")) {
// deal with multi-line values, skip strikethrough of value
description += strike("**default value:** ", depr) + "\n```\n" + defaultValue + "\n```\n";
} else if (prop.getType() == PropertyType.CLASSNAME
&& defaultValue.startsWith("org.apache.accumulo")) {
description += strike("**default value:** {% jlink -f " + defaultValue + " %}", depr);
} else {
description += strike("**default value:** `" + defaultValue + "`", depr);
}
}
doc.println("| " + key + " | " + description + " |");
}
Expand Down
24 changes: 20 additions & 4 deletions core/src/main/java/org/apache/accumulo/core/conf/Property.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public enum Property {
HDFS. To use the ChangeSecret tool, run the command: `./bin/accumulo \
admin changeSecret`.
""", "1.3.5"),
INSTANCE_VOLUMES("instance.volumes", "", PropertyType.VOLUMES, """
INSTANCE_VOLUMES("instance.volumes", null, PropertyType.VOLUMES, """
A comma separated list of dfs uris to use. Files will be stored across \
these filesystems. In some situations, the first volume in this list \
may be treated differently, such as being preferred for writing out \
Expand Down Expand Up @@ -1437,14 +1437,22 @@ start with the category prefix, followed by a scope (minc, majc, scan, \
private boolean isReplaced;
private Property replacedBy = null;
private final PropertyType type;
private final boolean isRequired;
// private final Predicate<String> checkValue;

Property(String name, String defaultValue, PropertyType type, String description,
String availableSince) {
this.key = name;
this.defaultValue = defaultValue;
this.type = type;
this.description = description;
this.availableSince = availableSince;
this.type = type;
isRequired = this.defaultValue == null;
/*
* checkValue = isRequired ? ((Predicate<String>)
* Objects::nonNull).and(Predicate.not(String::isEmpty)) .and(type::isValidFormat) :
* ((Predicate<String>) Objects::isNull).or(String::isEmpty).or(type::isValidFormat);
*/
}

@Override
Expand Down Expand Up @@ -1480,6 +1488,15 @@ public PropertyType getType() {
return this.type;
}

/**
* Gets isRequired of this property.
*
* @return isRequired
*/
public boolean getIsRequired() {
return this.isRequired;
}

/**
* Gets the description of this property.
*
Expand Down Expand Up @@ -1641,7 +1658,7 @@ public static boolean isValidProperty(final String key, final String value) {
// If a key doesn't exist yet, then check if it follows a valid prefix
return validPrefixes.stream().anyMatch(key::startsWith);
}
return (isValidPropertyKey(key) && p.getType().isValidFormat(value));
return isValidPropertyKey(key) && p.getType().isValidFormat(value);
}

/**
Expand All @@ -1653,7 +1670,6 @@ public static boolean isValidProperty(final String key, final String value) {
*/
public static boolean isValidPropertyKey(String key) {
return validProperties.contains(key) || validPrefixes.stream().anyMatch(key::startsWith);

}

/**
Expand Down
34 changes: 26 additions & 8 deletions core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import static java.util.Objects.requireNonNull;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Objects;
Expand Down Expand Up @@ -155,7 +157,7 @@ public enum PropertyType {
BOOLEAN("boolean", in(false, null, "true", "false"),
"Has a value of either 'true' or 'false' (case-insensitive)"),

URI("uri", x -> true, "A valid URI"),
URI("uri", new ValidUri(), "A valid URI"),

FILENAME_EXT("file name extension", in(true, RFile.EXTENSION),
"One of the currently supported filename extensions for storing table data files. "
Expand Down Expand Up @@ -247,12 +249,35 @@ public boolean test(String value) {
}
}

/**
* Validate that the provided string can be used to create a valid URI.
*/
private static class ValidUri implements Predicate<String> {
private static final Logger log = LoggerFactory.getLogger(ValidUri.class);

@Override
public boolean test(String uri) {
if (uri == null) {
return true;
}
try {
new URI(uri);
return true;
} catch (URISyntaxException e) {
log.error("provided uri string is not valid");
Comment thread
Amemeda marked this conversation as resolved.
return false;
}
}
}

private static class ValidVolumes implements Predicate<String> {
private static final Logger log = LoggerFactory.getLogger(ValidVolumes.class);

@Override
public boolean test(String volumes) {
if (volumes == null) {
return true;
} else if (volumes.isEmpty()) {
return false;
}
try {
Expand Down Expand Up @@ -306,7 +331,6 @@ public boolean test(String type) {
}
}
}

}

private static final Pattern SUFFIX_REGEX = Pattern.compile("\\D*$"); // match non-digits at end
Expand Down Expand Up @@ -413,14 +437,8 @@ public Matches(final Pattern pattern) {

@Override
public boolean test(final String input) {
// TODO when the input is null, it just means that the property wasn't set
// we can add checks for not null for required properties with
// Predicates.and(Predicates.notNull(), ...),
// or we can stop assuming that null is always okay for a Matches predicate, and do that
// explicitly with Predicates.or(Predicates.isNull(), ...)
Comment on lines -416 to -420

@ctubbsii ctubbsii Aug 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment that was removed suggests that we should not check input == null here, but should use Predicates.or(Predicates.isNull(), ...) for any patterns where we want to allow null.

If we leave the input == null here, then we need to do something like Predicates.and(Predicates.isNull().negate(), ...) for required properties.

I'm not sure if we've done either, or which would be easier to do if we haven't.

@Amemeda Amemeda Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this TODO with Dom, and he said the required properties check in ServerConfigCheckRunner is already doing this/ or something close

image

@ctubbsii ctubbsii Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the null is still being allowed here in the property type validation. That's kind of my point. We are allowing all nulls to pass through here, and then check them later. The comment that was removed was suggesting that we could do this better by disallowing nulls here.

Consider the following, which roughly represents what we have today:

// implied validation from the type
MY_PROP_ENUM("key", PropertyType.MyType, "description");

The problem here is that PropertyType.MyType.isValidFormat() must return true if it's null, even for required properties, because the type validation doesn't know if the property is required or not.

Consider this alternative instead:

// explicit validation from the type, with an optional nullable; type no longer has to allow nulls
// alternatively, the type always allows nulls, but we explicitly say that it's not null in the explicit validator
MY_PROP_ENUM("key", PropertyType.MyType, PropertyType.MyType::isValidFormat, "description");
MY_PROP_ENUM2("key2", PropertyType.MyType2, Predicate.isNull().or(PropertyType.MyType::isValidFormat), "description");

Alternatively:

// stored the required bit with the property
MY_PROP_ENUM("key", PropertyType.MyType, /* required = */ true, "description");
// modify the PropertyType.isValidFormat()
public boolean isValidFormat(String string, boolean required) {
  // ensure non-null in here before passing to the type-specific predicate to test the non-null format
}

I think the implication here is that the required set needs to be removed, and replaced with either explicit per-property validation, or an extra per-property "required" boolean parameter to track which properties allow null/empty string.

return input == null || pattern.matcher(input).matches();
}

}

public static class PortRange extends Matches {
Expand Down
26 changes: 14 additions & 12 deletions core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public void testProperties() {
for (Property prop : Property.values()) {
if (prop.getType().equals(PropertyType.PREFIX)) {
validPrefixes.add(prop.getKey());
// assertFalse(prop.getIsRequired());
}
}

Expand All @@ -63,18 +64,19 @@ public void testProperties() {
"PREFIX property " + prop.name() + " has unexpected non-null default value.");
} else {
// default values shouldn't be null, but they can be an empty string
assertNotNull(prop.getDefaultValue());
// default values shouldn't start or end with whitespace
assertEquals(prop.getDefaultValue().strip(), prop.getDefaultValue(),
"Property " + prop.name() + " starts or ends with whitespace");
// default values shouldn't contain newline characters or tabs
assertFalse(prop.getDefaultValue().contains("\t"),
"Property " + prop.name() + " contains a tab character");
assertFalse(prop.getDefaultValue().contains("\n"),
"Property " + prop.name() + " contains a newline (\\n) character");
assertFalse(prop.getDefaultValue().contains("\r"),
"Property " + prop.name() + " contains a return (\\r) character");

if (!prop.getIsRequired()) {
assertNotNull(prop.getDefaultValue());
// default values shouldn't start or end with whitespace
assertEquals(prop.getDefaultValue().strip(), prop.getDefaultValue(),
"Property " + prop.name() + " starts or ends with whitespace");
// default values shouldn't contain newline characters or tabs
assertFalse(prop.getDefaultValue().contains("\t"),
"Property " + prop.name() + " contains a tab character");
assertFalse(prop.getDefaultValue().contains("\n"),
"Property " + prop.name() + " contains a newline (\\n) character");
assertFalse(prop.getDefaultValue().contains("\r"),
"Property " + prop.name() + " contains a return (\\r) character");
}
assertTrue(Property.isValidProperty(prop.getKey(), prop.getDefaultValue()),
"Property " + prop.name() + " has invalid default value " + prop.getDefaultValue()
+ " for type " + prop.getType());
Expand Down
7 changes: 5 additions & 2 deletions core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,9 @@ public void testTypePREFIX() {
public void testTypeSTRING() {
valid(null, "", "whatever");
}
/*
* @Test public void testTypeNON_EMPTY_STRING() { valid("whatever"); invalid(null, "", " "); }
*/

@Test
public void testTypeTIMEDURATION() {
Expand All @@ -228,9 +231,9 @@ public void testTypeFILENAME_EXT() {
@Test
public void testTypeVOLUMES() {
// more comprehensive parsing tests are in ConfigurationTypeHelperTest.testGetVolumeUris()
valid("", "hdfs:/volA", ",hdfs:/volA", "hdfs:/volA,", "hdfs:/volA,file:/volB",
valid(null, "hdfs:/volA", ",hdfs:/volA", "hdfs:/volA,", "hdfs:/volA,file:/volB",
",hdfs:/volA,file:/volB", "hdfs:/volA,,file:/volB", "hdfs:/volA,file:/volB, ,");
invalid(null, " ", ",", ",,,", " ,,,", ",,, ", ", ,,", "hdfs:/volA,hdfs:/volB,volA",
invalid("", " ", ",", ",,,", " ,,,", ",,, ", ", ,,", "hdfs:/volA,hdfs:/volB,volA",
",volA,hdfs:/volA,hdfs:/volB", "hdfs:/volA,,volA,hdfs:/volB",
"hdfs:/volA,volA,hdfs:/volB, ,", "hdfs:/volA,hdfs:/volB,hdfs:/volA",
"hdfs:/volA,hdfs :/::/volB");
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public void testDefault() {
var conf = SiteConfiguration.empty().build();
assertEquals("localhost:2181", conf.get(Property.INSTANCE_ZK_HOST));
assertEquals("DEFAULT", conf.get(Property.INSTANCE_SECRET));
assertEquals("", conf.get(Property.INSTANCE_VOLUMES));
assertNull(conf.get(Property.INSTANCE_VOLUMES.getDefaultValue()));
assertEquals("120s", conf.get(Property.GENERAL_RPC_TIMEOUT));
assertEquals("1G", conf.get(Property.TSERV_WAL_MAX_SIZE));
assertEquals("org.apache.accumulo.core.spi.crypto.NoCryptoServiceFactory",
Expand Down
28 changes: 1 addition & 27 deletions ...e/src/main/java/org/apache/accumulo/server/util/checkCommand/ServerConfigCheckRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import java.util.HashMap;
import java.util.Map;
import java.util.Set;

import org.apache.accumulo.core.cli.ServerOpts;
import org.apache.accumulo.core.conf.Property;
Expand All @@ -47,34 +46,9 @@ public boolean runCheck(ServerContext context, ServerOpts opts, boolean fixFiles
var val = entry.getValue();
if (!Property.isValidProperty(key, val)) {
log.warn("Invalid property (key={} val={}) found in the config", key, val);
status &= false;
status = false;
}
}

log.trace("Checking that all required config properties are present");
// there are many properties that should be set (default value or user set), identifying them
// all and checking them here is unrealistic. Some property that is not set but is expected
// will likely result in some sort of failure eventually anyway. We will just check a few
// obvious required properties here.
Set<Property> requiredProps = Set.of(Property.INSTANCE_ZK_HOST, Property.INSTANCE_ZK_TIMEOUT,
Property.INSTANCE_SECRET, Property.INSTANCE_VOLUMES, Property.GENERAL_THREADPOOL_SIZE,
Property.GENERAL_DELEGATION_TOKEN_LIFETIME,
Property.GENERAL_DELEGATION_TOKEN_UPDATE_INTERVAL, Property.GENERAL_IDLE_PROCESS_INTERVAL,
Property.GENERAL_LOW_MEM_DETECTOR_INTERVAL, Property.GENERAL_LOW_MEM_DETECTOR_THRESHOLD,
Property.GENERAL_SERVER_LOCK_VERIFICATION_INTERVAL, Property.MANAGER_CLIENTPORT,
Property.TSERV_CLIENTPORT, Property.GC_CYCLE_START, Property.GC_CYCLE_DELAY,
Property.GC_PORT, Property.MONITOR_PORT, Property.TABLE_MAJC_RATIO,
Property.TABLE_SPLIT_THRESHOLD);
for (var reqProp : requiredProps) {
var confPropVal = config.get(reqProp);
// already checked that all set properties are valid, just check that it is set then we know
// it's valid
if (confPropVal == null || confPropVal.isEmpty()) {
log.warn("Required property {} is not set!", reqProp);
status &= false;
}
}

printCompleted(status);
return status;
}
Expand Down