Copilot commented on code in PR #3581:
URL: https://github.com/apache/celeborn/pull/3581#discussion_r2752774117
##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkUtils.java:
##########
@@ -228,17 +228,24 @@ public static <K, C> ShuffleReader<K, C> getReader(
public static final String COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS =
"org.apache.spark.shuffle.celeborn.ColumnarHashBasedShuffleWriter";
- static final DynConstructors.Builder
COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CONSTRUCTOR_BUILDER =
- DynConstructors.builder()
- .impl(
- COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS,
- int.class,
- CelebornShuffleHandle.class,
- TaskContext.class,
- CelebornConf.class,
- ShuffleClient.class,
- ShuffleWriteMetricsReporter.class,
- SendBufferPool.class);
+
+ /**
+ * Lazy holder for ColumnarHashBasedShuffleWriter constructor builder. The
builder is initialized
+ * only when this class is first accessed, ensuring lazy loading without
explicit synchronization.
+ */
+ private static class ColumnarHashBasedShuffleWriterConstructorBuilderHolder {
+ private static final DynConstructors.Builder INSTANCE =
+ DynConstructors.builder()
+ .impl(
+ COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS,
+ int.class,
+ CelebornShuffleHandle.class,
+ TaskContext.class,
+ CelebornConf.class,
+ ShuffleClient.class,
+ ShuffleWriteMetricsReporter.class,
+ SendBufferPool.class);
Review Comment:
The holder should store the built `DynConstructors.Ctor` instead of the
`DynConstructors.Builder`. This is inconsistent with the established pattern in
this file where all other dynamic reflection fields store the built result (see
GET_READER_METHOD at line 176, UnregisterAllMapAndMergeOutput_METHOD at line
312, and even CelebornSkewShuffleMethodHolder at line 563).
Change `INSTANCE` to `private static final DynConstructors.Ctor<?> INSTANCE`
and append `.build()` at the end of line 247. Then update line 258 to remove
the `.build()` call and invoke directly on INSTANCE. This avoids the
unnecessary method call overhead on every invocation and follows the
established convention.
##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkUtils.java:
##########
@@ -248,26 +255,33 @@ public static <K, V, C> HashBasedShuffleWriter<K, V, C>
createColumnarHashBasedS
ShuffleClient client,
ShuffleWriteMetricsReporter metrics,
SendBufferPool sendBufferPool) {
- return COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CONSTRUCTOR_BUILDER
+ return ColumnarHashBasedShuffleWriterConstructorBuilderHolder.INSTANCE
.build()
.invoke(null, shuffleId, handle, taskContext, conf, client, metrics,
sendBufferPool);
}
public static final String COLUMNAR_SHUFFLE_READER_CLASS =
"org.apache.spark.shuffle.celeborn.CelebornColumnarShuffleReader";
- static final DynConstructors.Builder
COLUMNAR_SHUFFLE_READER_CONSTRUCTOR_BUILDER =
- DynConstructors.builder()
- .impl(
- COLUMNAR_SHUFFLE_READER_CLASS,
- CelebornShuffleHandle.class,
- int.class,
- int.class,
- int.class,
- int.class,
- TaskContext.class,
- CelebornConf.class,
- ShuffleReadMetricsReporter.class,
- ExecutorShuffleIdTracker.class);
+
+ /**
+ * Lazy holder for CelebornColumnarShuffleReader constructor builder. The
builder is initialized
Review Comment:
The JavaDoc comment describes a "constructor builder" but should describe a
"constructor" for consistency with the recommended implementation. After
storing the built `DynConstructors.Ctor` instead of the builder (as suggested
in the comment for lines 271-283), update this comment to read "Lazy holder for
CelebornColumnarShuffleReader constructor. The constructor is initialized..."
to accurately reflect what is being stored.
```suggestion
* Lazy holder for CelebornColumnarShuffleReader constructor. The
constructor is initialized
```
##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkUtils.java:
##########
@@ -248,26 +255,33 @@ public static <K, V, C> HashBasedShuffleWriter<K, V, C>
createColumnarHashBasedS
ShuffleClient client,
ShuffleWriteMetricsReporter metrics,
SendBufferPool sendBufferPool) {
- return COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CONSTRUCTOR_BUILDER
+ return ColumnarHashBasedShuffleWriterConstructorBuilderHolder.INSTANCE
.build()
.invoke(null, shuffleId, handle, taskContext, conf, client, metrics,
sendBufferPool);
}
public static final String COLUMNAR_SHUFFLE_READER_CLASS =
"org.apache.spark.shuffle.celeborn.CelebornColumnarShuffleReader";
- static final DynConstructors.Builder
COLUMNAR_SHUFFLE_READER_CONSTRUCTOR_BUILDER =
- DynConstructors.builder()
- .impl(
- COLUMNAR_SHUFFLE_READER_CLASS,
- CelebornShuffleHandle.class,
- int.class,
- int.class,
- int.class,
- int.class,
- TaskContext.class,
- CelebornConf.class,
- ShuffleReadMetricsReporter.class,
- ExecutorShuffleIdTracker.class);
+
+ /**
+ * Lazy holder for CelebornColumnarShuffleReader constructor builder. The
builder is initialized
+ * only when this class is first accessed, ensuring lazy loading without
explicit synchronization.
+ */
+ private static class ColumnarShuffleReaderConstructorBuilderHolder {
+ private static final DynConstructors.Builder INSTANCE =
+ DynConstructors.builder()
+ .impl(
+ COLUMNAR_SHUFFLE_READER_CLASS,
+ CelebornShuffleHandle.class,
+ int.class,
+ int.class,
+ int.class,
+ int.class,
+ TaskContext.class,
+ CelebornConf.class,
+ ShuffleReadMetricsReporter.class,
+ ExecutorShuffleIdTracker.class);
Review Comment:
The holder should store the built `DynConstructors.Ctor` instead of the
`DynConstructors.Builder`. This is inconsistent with the established pattern in
this file where all other dynamic reflection fields store the built result (see
GET_READER_METHOD at line 176, UnregisterAllMapAndMergeOutput_METHOD at line
312, and even CelebornSkewShuffleMethodHolder at line 563).
Change `INSTANCE` to `private static final DynConstructors.Ctor<?> INSTANCE`
and append `.build()` at the end of line 283. Then update line 296 to remove
the `.build()` call and invoke directly on INSTANCE. This avoids the
unnecessary method call overhead on every invocation and follows the
established convention.
##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkUtils.java:
##########
@@ -228,17 +228,24 @@ public static <K, C> ShuffleReader<K, C> getReader(
public static final String COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS =
"org.apache.spark.shuffle.celeborn.ColumnarHashBasedShuffleWriter";
- static final DynConstructors.Builder
COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CONSTRUCTOR_BUILDER =
- DynConstructors.builder()
- .impl(
- COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS,
- int.class,
- CelebornShuffleHandle.class,
- TaskContext.class,
- CelebornConf.class,
- ShuffleClient.class,
- ShuffleWriteMetricsReporter.class,
- SendBufferPool.class);
+
+ /**
+ * Lazy holder for ColumnarHashBasedShuffleWriter constructor builder. The
builder is initialized
Review Comment:
The JavaDoc comment describes a "constructor builder" but should describe a
"constructor" for consistency with the recommended implementation. After
storing the built `DynConstructors.Ctor` instead of the builder (as suggested
in the comment for lines 237-247), update this comment to read "Lazy holder for
ColumnarHashBasedShuffleWriter constructor. The constructor is initialized..."
to accurately reflect what is being stored.
```suggestion
* Lazy holder for ColumnarHashBasedShuffleWriter constructor. The
constructor is initialized
```
##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkUtils.java:
##########
@@ -228,17 +228,24 @@ public static <K, C> ShuffleReader<K, C> getReader(
public static final String COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS =
"org.apache.spark.shuffle.celeborn.ColumnarHashBasedShuffleWriter";
- static final DynConstructors.Builder
COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CONSTRUCTOR_BUILDER =
- DynConstructors.builder()
- .impl(
- COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CLASS,
- int.class,
- CelebornShuffleHandle.class,
- TaskContext.class,
- CelebornConf.class,
- ShuffleClient.class,
- ShuffleWriteMetricsReporter.class,
- SendBufferPool.class);
+
+ /**
+ * Lazy holder for ColumnarHashBasedShuffleWriter constructor builder. The
builder is initialized
+ * only when this class is first accessed, ensuring lazy loading without
explicit synchronization.
+ */
+ private static class ColumnarHashBasedShuffleWriterConstructorBuilderHolder {
Review Comment:
The holder class is named
`ColumnarHashBasedShuffleWriterConstructorBuilderHolder`, but it actually holds
a `DynConstructors.Builder` instance, not a constructor. Consider renaming to
`ColumnarHashBasedShuffleWriterBuilderHolder` for clarity and accuracy, as
"ConstructorBuilder" is redundant when the type already indicates it's a
Builder for constructors.
##########
client-spark/spark-3/src/main/java/org/apache/spark/shuffle/celeborn/SparkUtils.java:
##########
@@ -248,26 +255,33 @@ public static <K, V, C> HashBasedShuffleWriter<K, V, C>
createColumnarHashBasedS
ShuffleClient client,
ShuffleWriteMetricsReporter metrics,
SendBufferPool sendBufferPool) {
- return COLUMNAR_HASH_BASED_SHUFFLE_WRITER_CONSTRUCTOR_BUILDER
+ return ColumnarHashBasedShuffleWriterConstructorBuilderHolder.INSTANCE
.build()
.invoke(null, shuffleId, handle, taskContext, conf, client, metrics,
sendBufferPool);
}
public static final String COLUMNAR_SHUFFLE_READER_CLASS =
"org.apache.spark.shuffle.celeborn.CelebornColumnarShuffleReader";
- static final DynConstructors.Builder
COLUMNAR_SHUFFLE_READER_CONSTRUCTOR_BUILDER =
- DynConstructors.builder()
- .impl(
- COLUMNAR_SHUFFLE_READER_CLASS,
- CelebornShuffleHandle.class,
- int.class,
- int.class,
- int.class,
- int.class,
- TaskContext.class,
- CelebornConf.class,
- ShuffleReadMetricsReporter.class,
- ExecutorShuffleIdTracker.class);
+
+ /**
+ * Lazy holder for CelebornColumnarShuffleReader constructor builder. The
builder is initialized
+ * only when this class is first accessed, ensuring lazy loading without
explicit synchronization.
+ */
+ private static class ColumnarShuffleReaderConstructorBuilderHolder {
Review Comment:
The holder class is named `ColumnarShuffleReaderConstructorBuilderHolder`,
but it actually holds a `DynConstructors.Builder` instance, not a constructor.
Consider renaming to `ColumnarShuffleReaderBuilderHolder` for clarity and
accuracy, as "ConstructorBuilder" is redundant when the type already indicates
it's a Builder for constructors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
