Skip to content

HDDS-14923. Enable UselessStringValueOf, UseStringBufferForStringAppends in PMD #10007

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

Draft
ptlrs wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

HDDS-14923. Enable UselessStringValueOf, UseStringBufferForStringAppends in PMD #10007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be20a33
Enable UselessStringValueOf in PMD
ptlrs Mar 29, 2026
6c7cfda
HDDS-14923. Use StringBuilder for string appends
ptlrs Mar 29, 2026
a4716a1
Merge remote-tracking branch 'refs/remotes/upstream/master' into HDDS…
ptlrs Apr 3, 2026
d1a0479
HDDS-14923. Refactor string appends to use StringBuilder
ptlrs Apr 3, 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
2 changes: 2 additions & 0 deletions dev-support/pmd/pmd-ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@
<rule ref="category/java/performance.xml/StringToString"/>
<rule ref="category/java/performance.xml/UseArraysAsList"/>
<rule ref="category/java/performance.xml/UseIndexOfChar"/>
<rule ref="category/java/performance.xml/UseStringBufferForStringAppends"/>
<rule ref="category/java/performance.xml/UseStringBufferLength"/>
<rule ref="category/java/performance.xml/UselessStringValueOf"/>

<rule ref="category/java/codestyle.xml/FieldDeclarationsShouldBeAtStartOfClass">
<properties>
Expand Down
13 changes: 7 additions & 6 deletions hadoop-hdds/cli-common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,22 +134,23 @@ protected PrintWriter err() {
}

private static String handleFileSystemException(FileSystemException e) {
String errorMessage = e.getMessage();
StringBuilder sb = new StringBuilder();
sb.append("Error: ");

// If reason is set, return the exception's message as it is.
// Otherwise, construct a custom message based on the type of exception
if (e.getReason() == null) {
if (e instanceof NoSuchFileException) {
errorMessage = "File not found: " + errorMessage;
sb.append("File not found: ");
} else if (e instanceof AccessDeniedException) {
errorMessage = "Access denied: " + errorMessage;
sb.append("Access denied: ");
} else if (e instanceof FileAlreadyExistsException) {
errorMessage = "File already exists: " + errorMessage;
sb.append("File already exists: ");
} else {
errorMessage = e.getClass().getSimpleName() + ": " + errorMessage;
sb.append(e.getClass().getSimpleName()).append(": ");
}
}

return "Error: " + errorMessage;
return sb.append(e.getMessage()).toString();
}
}
11 changes: 7 additions & 4 deletions hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,9 @@ protected XceiverClientSpi getClient(Pipeline pipeline, boolean topologyAware)

private String getPipelineCacheKey(Pipeline pipeline,
boolean topologyAware) {
String key = pipeline.getId().getId().toString() + pipeline.getType();
StringBuilder key = new StringBuilder()
.append(pipeline.getId().getId())
.append(pipeline.getType());
boolean isEC = pipeline.getType() == HddsProtos.ReplicationType.EC;
if (topologyAware || isEC) {
try {
Expand All @@ -183,7 +185,8 @@ private String getPipelineCacheKey(Pipeline pipeline,
// Standalone port is chosen since all datanodes should have a
// standalone port regardless of version and this port should not
// have any collisions.
key += closestNode.getHostName() + closestNode.getStandalonePort();
key.append(closestNode.getHostName())
.append(closestNode.getStandalonePort());
} catch (IOException e) {
LOG.error("Failed to get closest node to create pipeline cache key:" +
e.getMessage());
Expand All @@ -194,13 +197,13 @@ private String getPipelineCacheKey(Pipeline pipeline,
// Append user short name to key to prevent a different user
// from using same instance of xceiverClient.
try {
key += UserGroupInformation.getCurrentUser().getShortUserName();
key.append(UserGroupInformation.getCurrentUser().getShortUserName());
} catch (IOException e) {
LOG.error("Failed to get current user to create pipeline cache key:" +
e.getMessage());
}
}
return key;
return key.toString();
}

/**
Expand Down
8 changes: 4 additions & 4 deletions ...rc/main/java/org/apache/hadoop/ozone/client/io/ECBlockReconstructedStripeInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,13 +597,13 @@ protected void loadDataBuffersFromStream()
} catch (ExecutionException ee) {
boolean added = failedDataIndexes.add(index);
Throwable t = ee.getCause() != null ? ee.getCause() : ee;
String msg = "{}: error reading [{}]";
StringBuilder msg = new StringBuilder("{}: error reading [{}]");
if (added) {
msg += ", marked as failed";
msg.append(", marked as failed");
} else {
msg += ", already had failed"; // should not really happen
msg.append(", already had failed"); // should not really happen
}
LOG.info(msg, this, index, t);
LOG.info(msg.toString(), this, index, t);

exceptionOccurred = true;
} catch (InterruptedException ie) {
Expand Down
14 changes: 9 additions & 5 deletions ...op-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBCheckpointManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,20 @@ public RocksDBCheckpoint createCheckpoint(String parentDir, String name) {
try {
long currentTime = System.currentTimeMillis();

String checkpointDir = StringUtils.EMPTY;
StringBuilder checkpointDir = new StringBuilder();
if (StringUtils.isNotEmpty(checkpointNamePrefix)) {
checkpointDir += checkpointNamePrefix;
checkpointDir.append(checkpointNamePrefix);
}

if (name == null) {
name = "_" + RDB_CHECKPOINT_DIR_PREFIX + currentTime;
checkpointDir.append('_')
.append(RDB_CHECKPOINT_DIR_PREFIX)
.append(currentTime);
} else {
checkpointDir.append(name);
}
checkpointDir += name;

Path checkpointPath = Paths.get(parentDir, checkpointDir);
Path checkpointPath = Paths.get(parentDir, checkpointDir.toString());
Instant start = Instant.now();

// Flush the DB WAL and mem table.
Expand Down
5 changes: 3 additions & 2 deletions .../cli-admin/src/main/java/org/apache/hadoop/ozone/admin/nssummary/DiskUsageSubCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,14 @@ public Void call() throws Exception {
if (cnt >= limit) {
break;
}
String subPath = subPathDU.path("path").asText("");
StringBuilder sb = new StringBuilder(subPathDU.path("path").asText(""));
// differentiate key from other types
if (!subPathDU.path("isKey").asBoolean(false)) {
subPath += OM_KEY_PREFIX;
sb.append(OM_KEY_PREFIX);
}
Comment on lines +165 to 169
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Apr 3, 2026

Choose a reason for hiding this comment

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

I think PMD is really wrong here: previously the (internal) StringBuilder was created only conditionally, but now it is eagerly created. We can avoid that.

String pathValue = subPathDU.path("path").asText("");
boolean isDir = !subPathDU.path("isKey").asBoolean(false);
String subPath = isDir ? (pathValue + OM_KEY_PREFIX) : pathValue;

long size = subPathDU.path("size").asLong(-1);
long sizeWithReplica = subPathDU.path("sizeWithReplica").asLong(-1);
String subPath = sb.toString();
if (subPath.startsWith(seekStr)) {
printDURow(subPath, size, sizeWithReplica);
++cnt;
Expand Down
34 changes: 18 additions & 16 deletions ...one/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/om/ListOpenFilesSubCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,40 +142,42 @@ private void printOpenKeysList(ListOpenFilesResult res) {
for (OpenKeySession e : openFileList) {
long clientId = e.getId();
OmKeyInfo omKeyInfo = e.getKeyInfo();
String line = clientId + "\t" + Instant.ofEpochMilli(omKeyInfo.getCreationTime()) + "\t";
StringBuilder line = new StringBuilder()
.append(clientId).append('\t')
.append(Instant.ofEpochMilli(omKeyInfo.getCreationTime())).append('\t');

if (omKeyInfo.isHsync()) {
String hsyncClientIdStr =
omKeyInfo.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID);
long hsyncClientId = Long.parseLong(hsyncClientIdStr);
if (clientId == hsyncClientId) {
line += "Yes\t\t";
line.append("Yes\t\t");
} else {
// last hsync'ed with a different client ID than the client that
// initially opens the file (!)
line += "Yes w/ cid " + hsyncClientIdStr + "\t";
line.append("Yes w/ cid ").append(hsyncClientIdStr).append('\t');
}

if (showDeleted) {
if (omKeyInfo.getMetadata().containsKey(OzoneConsts.DELETED_HSYNC_KEY)) {
line += "Yes\t\t";
line.append("Yes\t\t");
} else {
line += "No\t\t";
line.append("No\t\t");
}
}
if (showOverwritten) {
if (omKeyInfo.getMetadata().containsKey(OzoneConsts.OVERWRITTEN_HSYNC_KEY)) {
line += "Yes\t";
line.append("Yes\t");
} else {
line += "No\t";
line.append("No\t");
}
}
} else {
line += showDeleted ? "No\t\tNo\t\t" : "No\t\t";
line += showOverwritten ? "No\t" : "";
line.append(showDeleted ? "No\t\tNo\t\t" : "No\t\t");
line.append(showOverwritten ? "No\t" : "");
}

line += getFullPathFromKeyInfo(omKeyInfo);
line.append(getFullPathFromKeyInfo(omKeyInfo));

System.out.println(line);
}
Expand Down Expand Up @@ -222,16 +224,16 @@ private String getMessageString(ListOpenFilesResult res, List<OpenKeySession> op
* @return the command to get the next batch of open keys
*/
private String getCmdForNextBatch(String lastElementFullPath) {
String nextBatchCmd = "ozone admin om lof " + omAddressOptions;
StringBuilder nextBatchCmd = new StringBuilder("ozone admin om lof ").append(omAddressOptions);
if (json) {
nextBatchCmd += " --json";
nextBatchCmd.append(" --json");
}
nextBatchCmd += " --length=" + limit;
nextBatchCmd.append(" --length=").append(limit);
if (pathPrefix != null && !pathPrefix.isEmpty()) {
nextBatchCmd += " --prefix=" + pathPrefix;
nextBatchCmd.append(" --prefix=").append(pathPrefix);
}
nextBatchCmd += " --start=" + lastElementFullPath;
return nextBatchCmd;
nextBatchCmd.append(" --start=").append(lastElementFullPath);
return nextBatchCmd.toString();
}

private String getFullPathFromKeyInfo(OmKeyInfo oki) {
Expand Down
10 changes: 5 additions & 5 deletions hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/keys/ListKeyHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,18 @@ private void listKeysInsideBucket(OzoneClient client, OzoneAddress address)
String volumeName = address.getVolumeName();
String bucketName = address.getBucketName();
String snapshotNameWithIndicator = address.getSnapshotNameWithIndicator();
String keyPrefix = "";
StringBuilder keyPrefix = new StringBuilder();

if (!Strings.isNullOrEmpty(snapshotNameWithIndicator)) {
keyPrefix += snapshotNameWithIndicator;
keyPrefix.append(snapshotNameWithIndicator);

if (!Strings.isNullOrEmpty(prefixFilter.getPrefix())) {
keyPrefix += "/";
keyPrefix.append('/');
}
}

if (!Strings.isNullOrEmpty(prefixFilter.getPrefix())) {
keyPrefix += prefixFilter.getPrefix();
keyPrefix.append(prefixFilter.getPrefix());
}

OzoneVolume vol = client.getObjectStore().getVolume(volumeName);
Expand All @@ -82,7 +82,7 @@ private void listKeysInsideBucket(OzoneClient client, OzoneAddress address)
bucket.setListCacheSize(maxKeyLimit);
}
Iterator<? extends OzoneKey> keyIterator = bucket.listKeys(
keyPrefix, listOptions.getStartItem());
keyPrefix.toString(), listOptions.getStartItem());

int counter = printAsJsonArray(keyIterator, maxKeyLimit);

Expand Down
6 changes: 3 additions & 3 deletions ...ne/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ public static OMPathInfoWithFSO verifyDirectoryKeysInPath(
List<OzoneAcl> acls = omBucketInfo.getAcls();

long lastKnownParentId = omBucketInfo.getObjectID();
String dbDirName = ""; // absolute path for trace logs
StringBuilder dbDirName = new StringBuilder(); // absolute path for trace logs
// for better logging
StringBuilder fullKeyPath = new StringBuilder(bucketKey);
while (elements.hasNext()) {
Expand All @@ -219,7 +219,7 @@ public static OMPathInfoWithFSO verifyDirectoryKeysInPath(
OmDirectoryInfo omDirInfo = omMetadataManager.getDirectoryTable().
get(dbNodeName);
if (omDirInfo != null) {
dbDirName += omDirInfo.getName() + OzoneConsts.OZONE_URI_DELIMITER;
dbDirName.append(omDirInfo.getName()).append(OzoneConsts.OZONE_URI_DELIMITER);
if (elements.hasNext()) {
result = OMDirectoryResult.DIRECTORY_EXISTS_IN_GIVENPATH;
lastKnownParentId = omDirInfo.getObjectID();
Expand Down Expand Up @@ -264,7 +264,7 @@ public static OMPathInfoWithFSO verifyDirectoryKeysInPath(
}

String dbDirKeyName = omMetadataManager.getOzoneDirKey(volumeName,
bucketName, dbDirName);
bucketName, dbDirName.toString());
LOG.trace("Acls from parent {} are : {}", dbDirKeyName, acls);

return new OMPathInfoWithFSO(leafNodeName, lastKnownParentId, missing,
Expand Down
17 changes: 10 additions & 7 deletions ...p-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/BucketHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,18 @@ public abstract OmDirectoryInfo getDirInfo(String[] names)
* @return subpath
*/
public static String buildSubpath(String path, String nextLevel) {
String subpath = path;
if (!subpath.startsWith(OM_KEY_PREFIX)) {
subpath = OM_KEY_PREFIX + subpath;
}
subpath = removeTrailingSlashIfNeeded(subpath);
path = !path.startsWith(OM_KEY_PREFIX)
? OM_KEY_PREFIX + path
: path;

StringBuilder sb = new StringBuilder(path)
.append(removeTrailingSlashIfNeeded(path));
Comment on lines +107 to +108
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai Apr 3, 2026

Choose a reason for hiding this comment

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

  • This duplicates path.
  • StringBuilder is unnecessary if nextLevel == null, and we can use concatenation in the other case
  • Better avoid reassigning parameter path.

So logic can be simplified:

String subpath = !subpath.startsWith(OM_KEY_PREFIX)
    ? OM_KEY_PREFIX + path
    : path;
subpath = removeTrailingSlashIfNeeded(subpath);
return nextLevel != null
    ? subpath + OM_KEY_PREFIX + nextLevel
    : subpath;


if (nextLevel != null) {
subpath = subpath + OM_KEY_PREFIX + nextLevel;
sb.append(nextLevel);
}
return subpath;

return sb.toString();
}

/**
Expand Down