Skip to content

Commit c83652b

Browse files
committed
Simplify: deduplicate sendControlMessage, HashMap over ConcurrentHashMap,
rename test Code review findings: - VeniceWriter: make 5-param sendControlMessage delegate to 6-param overload, eliminating duplicated getDebugInfo/isEndOfSegment/synchronized logic - MapLongAccumulator: ConcurrentHashMap → HashMap — Spark tasks are single-threaded (each gets a fresh copy()), no concurrent access occurs - MRJobCounterHelperTest: rename misleading test to reflect what it actually tests (counter retrieval round-trip, not Reporter-based increment)
1 parent 0acc746 commit c83652b

3 files changed

Lines changed: 9 additions & 22 deletions

File tree

clients/venice-push-job/src/main/java/com/linkedin/venice/spark/datawriter/task/MapLongAccumulator.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,23 @@
11
package com.linkedin.venice.spark.datawriter.task;
22

33
import java.util.Collections;
4+
import java.util.HashMap;
45
import java.util.Map;
5-
import java.util.concurrent.ConcurrentHashMap;
66
import org.apache.spark.SparkContext;
77
import org.apache.spark.util.AccumulatorV2;
88

99

1010
/**
1111
* A Spark accumulator that maintains per-key long counters, merging by summing values per key.
1212
* Used for tracking per-partition record counts during Spark-based push jobs.
13+
*
14+
* Thread safety: Spark creates a fresh copy per task via {@link #copy()}, so each task's
15+
* instance is single-threaded. A plain HashMap suffices — no concurrent access occurs.
1316
*/
1417
public class MapLongAccumulator extends AccumulatorV2<scala.Tuple2<Integer, Long>, Map<Integer, Long>> {
1518
private static final long serialVersionUID = 1L;
1619

17-
private final ConcurrentHashMap<Integer, Long> map = new ConcurrentHashMap<>();
20+
private final HashMap<Integer, Long> map = new HashMap<>();
1821

1922
MapLongAccumulator() {
2023
}

clients/venice-push-job/src/test/java/com/linkedin/venice/hadoop/mapreduce/counter/MRJobCounterHelperTest.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,7 @@ public void testGetPerPartitionRecordCountsEmpty() {
3333
}
3434

3535
@Test
36-
public void testIncrPartitionRecordCountWithMockReporter() {
37-
// Use a Counters-based approach to verify the increment path indirectly:
38-
// Increment via Reporter, then read back via Counters
36+
public void testGetPerPartitionRecordCountsWithMultipleIncrements() {
3937
Counters counters = new Counters();
4038
Counters.Counter counter = counters.findCounter("Per Partition Record Count", "3");
4139
counter.increment(10);

internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2277,23 +2277,7 @@ public CompletableFuture<PubSubProduceResult> sendControlMessage(
22772277
Map<String, String> debugInfo,
22782278
PubSubProducerCallback callback,
22792279
LeaderMetadataWrapper leaderMetadataWrapper) {
2280-
// Work around until we upgrade to a more modern Avro version which supports overriding the
2281-
// String implementation.
2282-
controlMessage.debugInfo = getDebugInfo(debugInfo);
2283-
boolean isEndOfSegment = ControlMessageType.valueOf(controlMessage).equals(ControlMessageType.END_OF_SEGMENT);
2284-
synchronized (this.partitionLocks[partition]) {
2285-
return sendMessage(
2286-
this::getControlMessageKey,
2287-
MessageType.CONTROL_MESSAGE,
2288-
controlMessage,
2289-
isEndOfSegment,
2290-
partition,
2291-
callback,
2292-
true,
2293-
leaderMetadataWrapper,
2294-
VENICE_DEFAULT_LOGICAL_TS,
2295-
EmptyPubSubMessageHeaders.SINGLETON);
2296-
}
2280+
return sendControlMessage(controlMessage, partition, debugInfo, callback, leaderMetadataWrapper, null);
22972281
}
22982282

22992283
public CompletableFuture<PubSubProduceResult> sendControlMessage(
@@ -2303,6 +2287,8 @@ public CompletableFuture<PubSubProduceResult> sendControlMessage(
23032287
PubSubProducerCallback callback,
23042288
LeaderMetadataWrapper leaderMetadataWrapper,
23052289
PubSubMessageHeaders headers) {
2290+
// Work around until we upgrade to a more modern Avro version which supports overriding the
2291+
// String implementation.
23062292
controlMessage.debugInfo = getDebugInfo(debugInfo);
23072293
boolean isEndOfSegment = ControlMessageType.valueOf(controlMessage).equals(ControlMessageType.END_OF_SEGMENT);
23082294
synchronized (this.partitionLocks[partition]) {

0 commit comments

Comments
 (0)