Skip to content

Commit

Permalink
Resolves FoundationDB#2733: Remove String.format calls
Browse files Browse the repository at this point in the history
This removes as many `String.format` calls as could be done. For the most part, these were just using `%s` or `%d`, which can be replaced with String concatenation. For error messages, an effort was made to move the interpolated data into log message keys where appropriate so that the error messages are static strings (as much as possible), which is useful when trying to group together related errors in a production environment.

This resolves FoundationDB#2733.
  • Loading branch information
alecgrieser committed May 22, 2024
1 parent ef0921f commit 6252eea
Show file tree
Hide file tree
Showing 78 changed files with 267 additions and 207 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import javax.tools.Diagnostic;
import java.io.IOException;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand Down Expand Up @@ -327,7 +328,7 @@ private void error(final Messager messager,
final String msg,
final Object... args) {
Objects.requireNonNull(messager).printMessage(Diagnostic.Kind.ERROR,
String.format(msg, args),
String.format(Locale.ROOT, msg, args),
e);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,9 @@ public Uid(final long part1, final long part2) {

@Override
public String toString() {
return String.format("%016x%016x", part1, part2);
String hexPart1 = Long.toHexString(part1);
String hexPart2 = Long.toHexString(part2);
return ("0".repeat(16 - hexPart1.length())) + hexPart1 + ("0".repeat(16 - hexPart2.length())) + hexPart2;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public static String loggable(@Nullable byte[] bytes) {
} else if (b == BACKSLASH_CHARACTER) {
sb.append("\\\\");
} else {
sb.append(String.format("\\x%02x", b));
sb.append("\\x").append(HEX_CHARS[b >>> 4]).append(HEX_CHARS[b & 0x0F]);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ void concurrentlyReadAndFillInGaps() {
byte[] key = new byte[]{(byte) i, 0x00};
Transaction tr = multi.get(i + 1);
boolean inARange = i % 2 != 0;
assertEquals(inARange, rs.contains(tr, key).join(), () -> String.format("key %s was in a range", ByteArrayUtil.printable(key)));
assertEquals(inARange, rs.contains(tr, key).join(), () -> "key " + ByteArrayUtil.printable(key) + " was in a range");
if (!inARange) {
// The check should conflict with the insert if (and only if) the key wasn't in a range
conflicts.add(i + 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,21 +350,12 @@ private void rankedSetOp(TransactionContext tc, RankedSet rs) {
.map(pk -> rs.count(tr, pk).join())
.mapToLong(Long::longValue).sum();
if (!(d > 0 && r >= r2 && r < r2 + d && r2 == r3)) {
throw new IllegalStateException(String.format("Rank Mismatch: Key=%s; d=%d, r=%d; r2=%d; r3=%d",
ByteArrayUtil.printable(k),
d,
r,
r2,
r3));
throw new IllegalStateException("Rank Mismatch: Key=" + ByteArrayUtil.printable(k) + "; d=" + d + "; r=" + r + "; r2=" + r2 + "; r3=" + r3);
}
} else {
long r3 = rs.getRangeList(tr, new byte[] {0x00}, k).size();
if (!(r == r2 && r2 == r3)) {
throw new IllegalStateException(String.format("Rank Mismatch: Key=%s; r=%d; r2=%d; r3=%d",
ByteArrayUtil.printable(k),
r,
r2,
r3));
throw new IllegalStateException("Rank Mismatch: Key=" + ByteArrayUtil.printable(k) + "; r=" + r + "; r2=" + r2 + "; r3=" + r3);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,13 @@ public void commit(Set<Integer> expectedConflicts) {
Transaction tr = transactions.get(i);
if (expectedConflicts.contains(i)) {
CompletionException err = assertThrows(CompletionException.class, () -> tr.commit().join(),
String.format("Transaction %d should not commit successfully", i));
"Transaction " + i + " should not commit successfully");
Throwable cause = err.getCause();
assertNotNull(cause);
assertTrue(cause instanceof FDBException);
assertEquals(FDBError.NOT_COMMITTED.code(), ((FDBException)cause).getCode());
} else {
assertDoesNotThrow(() -> tr.commit().join(), String.format("Transaction %d should commit successfully", i));
assertDoesNotThrow(() -> tr.commit().join(), "Transaction " + i + " should commit successfully");
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ public TestDatabaseExtension() {
public static int getAPIVersion() {
int apiVersion = Integer.parseInt(System.getProperty(API_VERSION_PROPERTY, "630"));
if (apiVersion < MIN_API_VERSION || apiVersion > MAX_API_VERSION) {
throw new IllegalStateException(String.format("unsupported API version %d (must be between %d and %d)",
apiVersion, MIN_API_VERSION, MAX_API_VERSION));
throw new IllegalStateException("unsupported API version " + apiVersion + " (must be between " + MIN_API_VERSION + " and " + MAX_API_VERSION + ")");
}
return apiVersion;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public long getBytesScanned() {

@Override
public String toString() {
return String.format("ByteScanLimiter(%d limit, %d left)", originalLimit, bytesRemaining.get());
return "ByteScanLimiter(" + originalLimit + " limit, " + bytesRemaining.get() + " left)";
}
}

Expand Down Expand Up @@ -154,7 +154,7 @@ public long getBytesScanned() {

@Override
public String toString() {
return String.format("ByteScanLimiter(UNLIMITED, %d scanned)", bytesScanned.get());
return "ByteScanLimiter(UNLIMITED, " + bytesScanned.get() + " scanned)";
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package com.apple.foundationdb.record;

import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.logging.LogMessageKeys;
import com.apple.foundationdb.record.query.plan.cascades.CorrelationIdentifier;
import com.apple.foundationdb.record.query.plan.cascades.typing.TypeRepository;

Expand Down Expand Up @@ -145,7 +146,8 @@ public Object getBinding(@Nonnull CorrelationIdentifier alias) {
public Object dereferenceConstant(@Nonnull final CorrelationIdentifier alias, @Nonnull final String constantId) {
final var constantsMap = (Map<String, ?>)bindings.get(Bindings.Internal.CONSTANT.bindingName(alias.getId()));
if (constantsMap == null) {
throw new RecordCoreException(String.format("could not find '%s'-'%s' in the evaluation context", alias, constantId));
throw new RecordCoreException("could not find constant in the evaluation context")
.addLogInfo(LogMessageKeys.KEY, "'" + alias.getId() + "' - '" + constantId + "'");
}
return constantsMap.get(constantId);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -377,19 +377,19 @@ public String toString() {
components.add(isolationLevel.toString());
}
if (skip != 0) {
components.add(String.format("skip %d", skip));
components.add("skip " + skip);
}
if (rowLimit != ReadTransaction.ROW_LIMIT_UNLIMITED) {
components.add(String.format("rowLimit %d", rowLimit));
components.add("rowLimit " + rowLimit);
}
if (timeLimit != UNLIMITED_TIME) {
components.add(String.format("timeLimit %d ms", timeLimit));
components.add("timeLimit " + timeLimit + " ms");
}
if (failOnScanLimitReached) {
components.add("fail on scan limit");
}
components.add(state.toString());
return String.format("ExecuteProperties(%s)", String.join(", ", components));
return "ExecuteProperties(" + String.join(", ", components) + ")";
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package com.apple.foundationdb.record;

import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.logging.LogMessageKeys;
import com.apple.foundationdb.record.metadata.FormerIndex;
import com.apple.foundationdb.record.metadata.Index;
import com.apple.foundationdb.record.metadata.JoinedRecordType;
Expand Down Expand Up @@ -579,7 +580,7 @@ private static void getDependencies(@Nonnull Descriptors.FileDescriptor fileDesc
allDependencies.put(dependency.getName(), dependency);
getDependencies(dependency, allDependencies, excludedDependencies);
} else if (!allDependencies.get(dependency.getName()).equals(dependency)) {
throw new MetaDataException(String.format("Dependency mismatch found for file %s", dependency.getName()));
throw new MetaDataException("Dependency mismatch found for file").addLogInfo(LogMessageKeys.VALUE, dependency.getName());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package com.apple.foundationdb.record;

import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.logging.LogMessageKeys;
import com.apple.foundationdb.record.metadata.FormerIndex;
import com.apple.foundationdb.record.metadata.Index;
import com.apple.foundationdb.record.metadata.IndexTypes;
Expand Down Expand Up @@ -592,7 +593,7 @@ private Descriptors.FileDescriptor[] getDependencies(@Nonnull DescriptorProtos.F
generatedDependencies.put(key, dependencies[index]);
} else {
// Unknown dependency.
throw new MetaDataException(String.format("Dependency %s not found", key));
throw new MetaDataException("Dependency not found").addLogInfo(LogMessageKeys.VALUE, key);
}
}
return dependencies;
Expand Down Expand Up @@ -735,8 +736,10 @@ private void updateUnionFieldsAndRecordTypes(@Nonnull Descriptors.Descriptor uni
// New field and record type.
RecordTypeBuilder recordType = processRecordType(unionField, processExtensionOptions);
if (recordType.getSinceVersion() != null && recordType.getSinceVersion() != version) {
throw new MetaDataException(String.format("Record type version (%d) does not match meta-data version (%d)",
recordType.getSinceVersion(), version));
throw new MetaDataException("Record type since version does not match meta-data version")
.addLogInfo(LogMessageKeys.META_DATA_VERSION, version)
.addLogInfo("since_version", recordType.getSinceVersion())
.addLogInfo(LogMessageKeys.RECORD_TYPE, recordType.getName());
} else {
recordType.setSinceVersion(version);
}
Expand Down Expand Up @@ -1310,8 +1313,9 @@ public RecordMetaDataBuilder setSubspaceKeyCounter(long subspaceKeyCounter) {
throw new MetaDataException("Counter-based subspace keys not enabled");
}
if (subspaceKeyCounter <= this.subspaceKeyCounter) {
throw new MetaDataException(String.format("Subspace key counter must be set to a value greater than its current value (%d)",
this.subspaceKeyCounter));
throw new MetaDataException("Subspace key counter must be set to a value greater than its current value")
.addLogInfo(LogMessageKeys.EXPECTED, "greater than " + this.subspaceKeyCounter)
.addLogInfo(LogMessageKeys.ACTUAL, subspaceKeyCounter);
}
this.subspaceKeyCounter = subspaceKeyCounter;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public int getRecordsScanned() {

@Override
public String toString() {
return String.format("RecordScanLimiter(%d limit, %d left)", originalLimit, allowedRecordScansRemaining.get());
return "RecordScanLimiter(" + originalLimit + " limit, " + allowedRecordScansRemaining.get() + " left)";
}
}

Expand Down Expand Up @@ -139,7 +139,7 @@ public int getRecordsScanned() {

@Override
public String toString() {
return String.format("RecordScanLimiter(UNLIMITED, %d scanned)", recordsScanned.get());
return "RecordScanLimiter(UNLIMITED, " + recordsScanned.get() + " scanned)";
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ public ScanProperties setStreamingMode(@Nonnull CursorStreamingMode cursorStream

@Override
public String toString() {
return String.format("ScanProperties(%s, direction: %s, streaming mode: %s)",
executeProperties, reverse ? "reverse" : "forward", cursorStreamingMode);
return "ScanProperties(" + executeProperties
+ ", direction: " + (reverse ? "reverse" : "forward")
+ ", streaming mode: " + cursorStreamingMode
+ ")";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.apple.foundationdb.annotation.API;
import com.apple.foundationdb.record.RecordCoreException;
import com.apple.foundationdb.record.RecordMetaDataProto;
import com.apple.foundationdb.record.logging.LogMessageKeys;
import com.apple.foundationdb.record.metadata.expressions.LiteralKeyExpression;
import com.apple.foundationdb.record.query.expressions.Comparisons;
import com.apple.foundationdb.record.query.plan.cascades.values.Value;
Expand Down Expand Up @@ -54,7 +55,7 @@ public static IndexComparison fromProto(@Nonnull final RecordMetaDataProto.Compa
} else if (proto.hasNullComparison()) {
return new NullComparison(proto.getNullComparison());
}
throw new RecordCoreException(String.format("attempt to deserialize unsupported comparison '%s'", proto));
throw new RecordCoreException("attempt to deserialize unsupported comparison").addLogInfo(LogMessageKeys.COMPARISON_VALUE, proto);
}

/**
Expand All @@ -72,7 +73,7 @@ public static IndexComparison fromComparison(@Nonnull final Comparisons.Comparis
} else if (comparison instanceof Comparisons.NullComparison) {
return new NullComparison((Comparisons.NullComparison)comparison);
} else {
throw new RecordCoreException(String.format("attempt to create PoJo index comparison from unsupported comparison '%s'", comparison));
throw new RecordCoreException("attempt to create PoJo index comparison from unsupported comparison").addLogInfo(LogMessageKeys.COMPARISON_VALUE, comparison);
}
}

Expand Down Expand Up @@ -160,7 +161,7 @@ public SimpleComparison(final Comparisons.SimpleComparison comparison) {
this.comparisonType = ComparisonType.IS_NULL;
break;
default:
throw new RecordCoreException(String.format("attempt to construct PoJo index comparison from unsupported comparison type '%s'", comparison.getType()));
throw new RecordCoreException("attempt to construct PoJo index comparison from unsupported comparison type").addLogInfo(LogMessageKeys.COMPARISON_TYPE, comparison.getType());
}
this.operand = comparison.getComparand(null, null);
}
Expand Down Expand Up @@ -199,7 +200,7 @@ public SimpleComparison(@Nonnull final RecordMetaDataProto.SimpleComparison prot
comparisonType = ComparisonType.IS_NULL;
break;
default:
throw new RecordCoreException(String.format("attempt to deserialize unsupported comparison type '%s'", proto.getType()));
throw new RecordCoreException("attempt to deserialize unsupported comparison type").addLogInfo(LogMessageKeys.COMPARISON_TYPE, proto.getType());
}
this.comparisonType = comparisonType;
this.operand = comparand;
Expand Down Expand Up @@ -245,7 +246,7 @@ public RecordMetaDataProto.Comparison toProto() {
protoComparison = RecordMetaDataProto.ComparisonType.IS_NULL;
break;
default:
throw new RecordCoreException(String.format("serialising comparison type '%s' is not supported", comparisonType));
throw new RecordCoreException("serialising comparison type is not supported").addLogInfo(LogMessageKeys.COMPARISON_TYPE, comparisonType);
}
return RecordMetaDataProto.Comparison.newBuilder().setSimpleComparison(RecordMetaDataProto.SimpleComparison.newBuilder()
.setType(protoComparison)
Expand Down Expand Up @@ -284,7 +285,7 @@ public Comparisons.Comparison toComparison() {
type = Comparisons.Type.IS_NULL;
break;
default:
throw new RecordCoreException(String.format("serialising comparison type '%s' is not supported", comparisonType));
throw new RecordCoreException("serializing comparison type is not supported").addLogInfo(LogMessageKeys.COMPARISON_TYPE, comparisonType);
}
return new Comparisons.SimpleComparison(type, operand);
}
Expand Down
Loading

0 comments on commit 6252eea

Please sign in to comment.