This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
The following commit(s) were added to refs/heads/master by this push:
new 5eb27588 Fix some bad practices reported by spotbugs (#177)
5eb27588 is described below
commit 5eb27588f28d31c68d9eff2b9da6881c2dcada46
Author: Kaijie Chen <[email protected]>
AuthorDate: Tue Aug 30 11:43:10 2022 +0800
Fix some bad practices reported by spotbugs (#177)
### What changes were proposed in this pull request?
Fix some bad practices reported by spotbugs (medium priority).
### Why are the changes needed?
To improve code quality.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
CI with extra spotbugs rules:
https://github.com/kaijchen/incubator-uniffle/actions/runs/2902861660
---
.../spark/shuffle/writer/WriteBufferManager.java | 2 +-
.../org/apache/spark/shuffle/RssShuffleManager.java | 4 ++--
.../apache/uniffle/common/config/ConfigOption.java | 2 +-
.../org/apache/uniffle/common/config/RssConf.java | 1 -
.../apache/uniffle/common/util/UnitConverter.java | 20 ++++++++++++++------
.../PartitionBalanceAssignmentStrategy.java | 2 +-
.../apache/uniffle/server/LocalStorageChecker.java | 4 +++-
.../apache/uniffle/server/ShuffleFlushManager.java | 6 ++++--
.../uniffle/server/buffer/ShuffleBufferManager.java | 4 ++--
.../apache/uniffle/storage/common/LocalStorage.java | 4 +++-
.../uniffle/storage/common/LocalStorageMeta.java | 4 ++--
11 files changed, 33 insertions(+), 20 deletions(-)
diff --git
a/client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java
b/client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java
index 3f408ebc..ffb6000b 100644
---
a/client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java
+++
b/client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java
@@ -184,7 +184,7 @@ public class WriteBufferManager extends MemoryConsumer {
// it's run in single thread, and is not thread safe
private int getNextSeqNo(int partitionId) {
- partitionToSeqNo.putIfAbsent(partitionId, new Integer(0));
+ partitionToSeqNo.putIfAbsent(partitionId, 0);
int seqNo = partitionToSeqNo.get(partitionId);
partitionToSeqNo.put(partitionId, seqNo + 1);
return seqNo;
diff --git
a/client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java
b/client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java
index 96ed7370..af5b36ac 100644
---
a/client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java
+++
b/client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java
@@ -639,7 +639,7 @@ public class RssShuffleManager implements ShuffleManager {
return result;
}
- class ReadMetrics extends ShuffleReadMetrics {
+ static class ReadMetrics extends ShuffleReadMetrics {
private ShuffleReadMetricsReporter reporter;
ReadMetrics(ShuffleReadMetricsReporter reporter) {
@@ -662,7 +662,7 @@ public class RssShuffleManager implements ShuffleManager {
}
}
- class WriteMetrics extends ShuffleWriteMetrics {
+ static class WriteMetrics extends ShuffleWriteMetrics {
private ShuffleWriteMetricsReporter reporter;
WriteMetrics(ShuffleWriteMetricsReporter reporter) {
diff --git
a/common/src/main/java/org/apache/uniffle/common/config/ConfigOption.java
b/common/src/main/java/org/apache/uniffle/common/config/ConfigOption.java
index c3693dac..7f420d71 100644
--- a/common/src/main/java/org/apache/uniffle/common/config/ConfigOption.java
+++ b/common/src/main/java/org/apache/uniffle/common/config/ConfigOption.java
@@ -152,7 +152,7 @@ public class ConfigOption<T> {
public boolean equals(Object o) {
if (this == o) {
return true;
- } else if (o != null && o.getClass() == ConfigOption.class) {
+ } else if (o != null && o.getClass() == this.getClass()) {
ConfigOption<?> that = (ConfigOption<?>) o;
return this.key.equals(that.key)
&& (this.defaultValue == null ? that.defaultValue == null :
diff --git a/common/src/main/java/org/apache/uniffle/common/config/RssConf.java
b/common/src/main/java/org/apache/uniffle/common/config/RssConf.java
index 57ae03c2..9e1c1f78 100644
--- a/common/src/main/java/org/apache/uniffle/common/config/RssConf.java
+++ b/common/src/main/java/org/apache/uniffle/common/config/RssConf.java
@@ -137,7 +137,6 @@ public class RssConf implements Cloneable {
* @return the (default) value associated with the given config option
*/
public int getInteger(ConfigOption<Integer> configOption) {
- Optional<Integer> a = getOptional(configOption);
return getOptional(configOption)
.orElseGet(configOption::defaultValue);
}
diff --git
a/common/src/main/java/org/apache/uniffle/common/util/UnitConverter.java
b/common/src/main/java/org/apache/uniffle/common/util/UnitConverter.java
index 97cc7697..77f6e0eb 100644
--- a/common/src/main/java/org/apache/uniffle/common/util/UnitConverter.java
+++ b/common/src/main/java/org/apache/uniffle/common/util/UnitConverter.java
@@ -87,11 +87,15 @@ public class UnitConverter {
long val = Long.parseLong(m.group(1));
String suffix = m.group(2);
// Check for invalid suffixes
- if (suffix != null && !byteSuffixes.containsKey(suffix)) {
- throw new NumberFormatException("Invalid suffix: \"" + suffix +
"\"");
+ ByteUnit byteUnit = unit;
+ if (suffix != null) {
+ byteUnit = byteSuffixes.get(suffix);
+ if (byteUnit == null) {
+ throw new NumberFormatException("Invalid suffix: \"" + suffix +
"\"");
+ }
}
// If suffix is valid use that, otherwise none was provided and use
the default passed
- return unit.convertFrom(val, suffix != null ? byteSuffixes.get(suffix)
: unit);
+ return unit.convertFrom(val, byteUnit);
} else if (fractionMatcher.matches()) {
throw new NumberFormatException("Fractional values are not supported.
Input was: "
+ fractionMatcher.group(1));
@@ -162,13 +166,17 @@ public class UnitConverter {
long val = Long.parseLong(m.group(1));
String suffix = m.group(2);
+ TimeUnit timeUnit = unit;
// Check for invalid suffixes
- if (suffix != null && !timeSuffixes.containsKey(suffix)) {
- throw new NumberFormatException("Invalid suffix: \"" + suffix + "\"");
+ if (suffix != null) {
+ timeUnit = timeSuffixes.get(suffix);
+ if (timeUnit == null) {
+ throw new NumberFormatException("Invalid suffix: \"" + suffix +
"\"");
+ }
}
// If suffix is valid use that, otherwise none was provided and use the
default passed
- return unit.convert(val, suffix != null ? timeSuffixes.get(suffix) :
unit);
+ return unit.convert(val, timeUnit);
} catch (NumberFormatException e) {
String timeError = "Time must be specified as seconds (s), "
+ "milliseconds (ms), microseconds (us), minutes (m or min), hour
(h), or day (d). "
diff --git
a/coordinator/src/main/java/org/apache/uniffle/coordinator/PartitionBalanceAssignmentStrategy.java
b/coordinator/src/main/java/org/apache/uniffle/coordinator/PartitionBalanceAssignmentStrategy.java
index d074b8cd..b915f317 100644
---
a/coordinator/src/main/java/org/apache/uniffle/coordinator/PartitionBalanceAssignmentStrategy.java
+++
b/coordinator/src/main/java/org/apache/uniffle/coordinator/PartitionBalanceAssignmentStrategy.java
@@ -100,7 +100,7 @@ public class PartitionBalanceAssignmentStrategy implements
AssignmentStrategy {
PartitionAssignmentInfo partitionInfo2 = serverToPartitions.get(o2);
double v1 = o1.getAvailableMemory() * 1.0 /
(partitionInfo1.getPartitionNum() + assignPartitions);
double v2 = o2.getAvailableMemory() * 1.0 /
(partitionInfo2.getPartitionNum() + assignPartitions);
- return -Double.compare(v1, v2);
+ return Double.compare(v2, v1);
}
});
diff --git
a/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java
b/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java
index b24e7e89..c0163b39 100644
--- a/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java
+++ b/server/src/main/java/org/apache/uniffle/server/LocalStorageChecker.java
@@ -151,7 +151,9 @@ public class LocalStorageChecker extends Checker {
}
File checkDir = new File(storageDir, "check");
try {
- checkDir.mkdirs();
+ if (!checkDir.mkdirs()) {
+ return false;
+ }
File writeFile = new File(checkDir, "test");
if (!writeFile.createNewFile()) {
return false;
diff --git
a/server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java
b/server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java
index aa67d036..daae92df 100644
--- a/server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java
+++ b/server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java
@@ -135,7 +135,9 @@ public class ShuffleFlushManager {
}
public void addToFlushQueue(ShuffleDataFlushEvent event) {
- flushQueue.offer(event);
+ if (!flushQueue.offer(event)) {
+ LOG.warn("Flush queue is full, discard event: " + event);
+ }
}
private void flushToFile(ShuffleDataFlushEvent event) {
@@ -335,7 +337,7 @@ public class ShuffleFlushManager {
}
}
- private class PendingShuffleFlushEvent {
+ private static class PendingShuffleFlushEvent {
private final ShuffleDataFlushEvent event;
private final long createTimeStamp = System.currentTimeMillis();
diff --git
a/server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java
b/server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java
index 660cd5c5..b4d55c07 100644
---
a/server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java
+++
b/server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java
@@ -74,9 +74,9 @@ public class ShuffleBufferManager {
this.shuffleFlushManager = shuffleFlushManager;
this.bufferPool = new ConcurrentHashMap<>();
this.retryNum =
conf.getInteger(ShuffleServerConf.SERVER_MEMORY_REQUEST_RETRY_MAX);
- this.highWaterMark = (long)(capacity / 100
+ this.highWaterMark = (long)(capacity / 100.0
*
conf.get(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_HIGHWATERMARK_PERCENTAGE));
- this.lowWaterMark = (long)(capacity / 100
+ this.lowWaterMark = (long)(capacity / 100.0
*
conf.get(ShuffleServerConf.SERVER_MEMORY_SHUFFLE_LOWWATERMARK_PERCENTAGE));
this.bufferFlushEnabled =
conf.getBoolean(ShuffleServerConf.SINGLE_BUFFER_FLUSH_ENABLED);
this.bufferFlushThreshold =
conf.getLong(ShuffleServerConf.SINGLE_BUFFER_FLUSH_THRESHOLD);
diff --git
a/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
b/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
index 0fce345f..a2fa0847 100644
--- a/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
+++ b/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorage.java
@@ -70,7 +70,9 @@ public class LocalStorage extends AbstractStorage {
File baseFolder = new File(basePath);
try {
FileUtils.deleteDirectory(baseFolder);
- baseFolder.mkdirs();
+ if (!baseFolder.mkdirs()) {
+ throw new IOException("Failed to create base folder: " + basePath);
+ }
} catch (IOException ioe) {
LOG.warn("Init base directory " + basePath + " fail, the disk should be
corrupted", ioe);
throw new RuntimeException(ioe);
diff --git
a/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorageMeta.java
b/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorageMeta.java
index a27136b5..07513981 100644
---
a/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorageMeta.java
+++
b/storage/src/main/java/org/apache/uniffle/storage/common/LocalStorageMeta.java
@@ -58,7 +58,7 @@ public class LocalStorageMeta {
shuffleMetaList.sort((Entry<String, ShuffleMeta> o1, Entry<String,
ShuffleMeta> o2) -> {
long sz1 = o1.getValue().getSize().longValue();
long sz2 = o2.getValue().getSize().longValue();
- return -Long.compare(sz1, sz2);
+ return Long.compare(sz2, sz1);
});
return shuffleMetaList
@@ -210,7 +210,7 @@ public class LocalStorageMeta {
}
// Consider that ShuffleMeta is a simple class, we keep the class
ShuffleMeta as an inner class.
- private class ShuffleMeta {
+ private static class ShuffleMeta {
private final AtomicLong size = new AtomicLong(0);
private final RoaringBitmap partitionBitmap = RoaringBitmap.bitmapOf();
private final AtomicLong uploadedSize = new AtomicLong(0);
