This is an automated email from the ASF dual-hosted git repository.
zhouky pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/incubator-celeborn.git
The following commit(s) were added to refs/heads/main by this push:
new 445143233 [CELEBORN-1072] Fix misc error prone reports found
445143233 is described below
commit 44514323332f2848fa5832669f98e2362128d738
Author: Mridul Muralidharan <mridulatgmail.com>
AuthorDate: Sun Oct 22 22:31:06 2023 +0800
[CELEBORN-1072] Fix misc error prone reports found
### What changes were proposed in this pull request?
Fix misc error prone reports.
As detailed in the jira, they are:
* Reference equality of boxed primitive types: see
[BoxedPrimitiveEquality](https://errorprone.info/bugpattern/BoxedPrimitiveEquality)
* Calling run directly - since use is legitimate, mark it as ignore. See:
[DoNotCall](https://errorprone.info/bugpattern/DoNotCall)
* `Ignore` test instead of catching `AssertionError` and ignoring it. See:
[AssertionFailureIgnored](https://errorprone.info/bugpattern/AssertionFailureIgnored)
### Why are the changes needed?
Fix misc error prone reports.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit tests
Closes #2019 from mridulm/fix-misc-issues.
Authored-by: Mridul Muralidharan <mridulatgmail.com>
Signed-off-by: zky.zhoukeyong <[email protected]>
---
.../spark/shuffle/celeborn/PackedRecordPointerSuiteJ.java | 13 +++++--------
.../celeborn/common/network/protocol/RegionStart.java | 6 +++---
.../common/network/TransportClientFactorySuiteJ.java | 1 +
.../celeborn/service/deploy/worker/PushDataHandler.scala | 4 ++--
4 files changed, 11 insertions(+), 13 deletions(-)
diff --git
a/client-spark/common/src/test/java/org/apache/spark/shuffle/celeborn/PackedRecordPointerSuiteJ.java
b/client-spark/common/src/test/java/org/apache/spark/shuffle/celeborn/PackedRecordPointerSuiteJ.java
index 5a5c40459..53aa26921 100644
---
a/client-spark/common/src/test/java/org/apache/spark/shuffle/celeborn/PackedRecordPointerSuiteJ.java
+++
b/client-spark/common/src/test/java/org/apache/spark/shuffle/celeborn/PackedRecordPointerSuiteJ.java
@@ -25,6 +25,7 @@ import java.io.IOException;
import org.apache.spark.SparkConf;
import org.apache.spark.memory.*;
import org.apache.spark.unsafe.memory.MemoryBlock;
+import org.junit.Ignore;
import org.junit.Test;
public class PackedRecordPointerSuiteJ {
@@ -80,16 +81,12 @@ public class PackedRecordPointerSuiteJ {
assertEquals(MAXIMUM_PARTITION_ID, packedPointer.getPartitionId());
}
- @Test
+ @Ignore
public void
partitionIdsGreaterThanMaximumPartitionIdWillOverflowOrTriggerError() {
PackedRecordPointer packedPointer = new PackedRecordPointer();
- try {
- // Pointers greater than the maximum partition ID will overflow or
trigger an assertion error
- packedPointer.set(PackedRecordPointer.packPointer(0,
MAXIMUM_PARTITION_ID + 1));
- assertFalse(MAXIMUM_PARTITION_ID + 1 == packedPointer.getPartitionId());
- } catch (AssertionError e) {
- // pass
- }
+ // Pointers greater than the maximum partition ID will overflow or trigger
an assertion error
+ packedPointer.set(PackedRecordPointer.packPointer(0, MAXIMUM_PARTITION_ID
+ 1));
+ assertFalse(MAXIMUM_PARTITION_ID + 1 == packedPointer.getPartitionId());
}
@Test
diff --git
a/common/src/main/java/org/apache/celeborn/common/network/protocol/RegionStart.java
b/common/src/main/java/org/apache/celeborn/common/network/protocol/RegionStart.java
index 4081c5cf4..2c327c880 100644
---
a/common/src/main/java/org/apache/celeborn/common/network/protocol/RegionStart.java
+++
b/common/src/main/java/org/apache/celeborn/common/network/protocol/RegionStart.java
@@ -29,8 +29,8 @@ public final class RegionStart extends RequestMessage {
public final String shuffleKey;
public final String partitionUniqueId;
public final int attemptId;
- public int currentRegionIndex;
- public Boolean isBroadcast;
+ public final int currentRegionIndex;
+ public final boolean isBroadcast;
public RegionStart(
byte mode,
@@ -38,7 +38,7 @@ public final class RegionStart extends RequestMessage {
String partitionUniqueId,
int attemptId,
int currentRegionIndex,
- Boolean isBroadcast) {
+ boolean isBroadcast) {
this.mode = mode;
this.shuffleKey = shuffleKey;
this.partitionUniqueId = partitionUniqueId;
diff --git
a/common/src/test/java/org/apache/celeborn/common/network/TransportClientFactorySuiteJ.java
b/common/src/test/java/org/apache/celeborn/common/network/TransportClientFactorySuiteJ.java
index 89db68998..022af8178 100644
---
a/common/src/test/java/org/apache/celeborn/common/network/TransportClientFactorySuiteJ.java
+++
b/common/src/test/java/org/apache/celeborn/common/network/TransportClientFactorySuiteJ.java
@@ -62,6 +62,7 @@ public class TransportClientFactorySuiteJ {
*
* <p>If concurrent is true, create multiple threads to create clients in
parallel.
*/
+ @SuppressWarnings("DoNotCall")
private void testClientReuse(int maxConnections, boolean concurrent)
throws IOException, InterruptedException {
diff --git
a/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala
b/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala
index 62edc2a93..a1bf40f57 100644
---
a/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala
+++
b/worker/src/main/scala/org/apache/celeborn/service/deploy/worker/PushDataHandler.scala
@@ -1043,7 +1043,7 @@ class PushDataHandler extends BaseMessageHandler with
Logging {
numPartitions,
bufferSize)
case Type.REGION_START =>
- val (currentRegionIndex, isBroadcast) =
+ val (currentRegionIndex, isBroadcast: Boolean) =
if (isLegacy)
(
msg.asInstanceOf[RegionStart].currentRegionIndex,
@@ -1051,7 +1051,7 @@ class PushDataHandler extends BaseMessageHandler with
Logging {
else
(
pbMsg.asInstanceOf[PbRegionStart].getCurrentRegionIndex,
- Boolean.box(pbMsg.asInstanceOf[PbRegionStart].getIsBroadcast))
+ pbMsg.asInstanceOf[PbRegionStart].getIsBroadcast)
fileWriter.asInstanceOf[MapPartitionFileWriter].regionStart(
currentRegionIndex,
isBroadcast)
