Use RemoveSafeDeltaCounterCell for PerWorker counters

runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/RemoveSafeDeltaCounterCell.java

@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.runners.dataflow.worker;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.beam.runners.core.metrics.CounterCell;
+import org.apache.beam.runners.core.metrics.DirtyState;
+import org.apache.beam.runners.core.metrics.MetricCell;
+import org.apache.beam.sdk.metrics.Counter;
+import org.apache.beam.sdk.metrics.MetricName;
+
+/**
+ * Version of {@link CounterCell} supporting multi-thread safe mutations and extraction of delta
+ * values. {@link RemoveSafeDeltaCounterCell} allows safe deletions from the underlying {@code
+ * countersMap} through the {@code deleteIfZero} method.
+ */
+public class RemoveSafeDeltaCounterCell implements Counter, MetricCell<Long> {
+
+  private final MetricName metricName;
+  /**
+   * This class does not own {@code countersMap} and only operates on a single key in the map
+   * specified by {@code metricName}. These opeations include the {@link Counter} interface along
+   * with the {@code deleteIfZero} method.
+   */
+  private final ConcurrentHashMap<MetricName, AtomicLong> countersMap;
+
+  /**
+   * @param metricName Specifies which metric this counter refers to.
+   * @param countersMap The underlying {@code map} used to store this metric.
+   */
+  public RemoveSafeDeltaCounterCell(
+      MetricName metricName, ConcurrentHashMap<MetricName, AtomicLong> countersMap) {
+    this.metricName = metricName;
+    this.countersMap = countersMap;
+  }
+
+  @Override
+  public void reset() {
+    countersMap.computeIfPresent(
+        metricName,
+        (unusedName, value) -> {
+          value.set(0);
+          return value;
+        });
+  }
+
+  @Override
+  public void inc(long n) {
+    countersMap.compute(
+        metricName,
+        (name, value) -> {
+          if (value == null) {
+            return new AtomicLong(n);
+          }
+          value.addAndGet(n);
+          return value;
+        });
+  }
+
+  @Override
+  public void inc() {
+    inc(1);
+  }
+
+  @Override
+  public void dec() {
+    inc(-1);
+  }
+
+  @Override
+  public void dec(long n) {
+    inc(-1 * n);
+  }
+
+  @Override
+  public MetricName getName() {
+    return metricName;
+  }
+
+  @Override
+  public DirtyState getDirty() {
+    throw new UnsupportedOperationException(
+        String.format("%s doesn't support the getDirty", getClass().getSimpleName()));
+  }
+
+  @Override
+  public Long getCumulative() {
+    throw new UnsupportedOperationException("getCumulative is not supported by Streaming Metrics");
+  }
+
+  /** Remove the metric from the {@code countersMap} if the the metric is zero valued. */
+  @SuppressWarnings(
+      "nullness") // computeIfPresent's remapping function can return null to remove the entry.
+  public void deleteIfZero() {
+    countersMap.computeIfPresent(
+        metricName,
+        (name, value) -> {
+          return value.get() == 0L ? null : value;
+        });
+  }
+}

runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingStepMetricsContainer.java

@@ -21,6 +21,8 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
 import javax.annotation.Nonnull;
 import org.apache.beam.runners.core.metrics.DistributionData;
 import org.apache.beam.runners.core.metrics.GaugeCell;
@@ -56,8 +58,7 @@ public class StreamingStepMetricsContainer implements MetricsContainer {
   private MetricsMap<MetricName, DeltaCounterCell> counters =
       new MetricsMap<>(DeltaCounterCell::new);
 
-  private MetricsMap<MetricName, DeltaCounterCell> perWorkerCounters =
-      new MetricsMap<>(DeltaCounterCell::new);
+  private ConcurrentHashMap<MetricName, AtomicLong> perWorkerCounters = new ConcurrentHashMap<>();
 
   private MetricsMap<MetricName, GaugeCell> gauges = new MetricsMap<>(GaugeCell::new);
 
@@ -88,7 +89,7 @@ public Counter getCounter(MetricName metricName) {
   @Override
   public Counter getPerWorkerCounter(MetricName metricName) {
     if (enablePerWorkerMetrics) {
-      return perWorkerCounters.get(metricName);
+      return new RemoveSafeDeltaCounterCell(metricName, perWorkerCounters);
     } else {
       return MetricsContainer.super.getPerWorkerCounter(metricName);
     }

runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/RemoveSafeDeltaCounterCellTest.java

@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.runners.dataflow.worker;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import org.apache.beam.sdk.metrics.MetricName;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link RemoveSafeDeltaCounterCell}. */
+@RunWith(JUnit4.class)
+public class RemoveSafeDeltaCounterCellTest {
+
+  @Test
+  public void testRemoveSafeDeltaCounterCell_deleteZero() throws Exception {
+    ConcurrentHashMap<MetricName, AtomicLong> metric_map = new ConcurrentHashMap<>();
+    MetricName metric1 = MetricName.named("namespace", "name_1");
+
+    RemoveSafeDeltaCounterCell cell_1 = new RemoveSafeDeltaCounterCell(metric1, metric_map);
+    RemoveSafeDeltaCounterCell cell_2 = new RemoveSafeDeltaCounterCell(metric1, metric_map);
+
+    cell_2.inc(10);
+    assertThat(metric_map.get(metric1).get(), equalTo(10L));
+
+    cell_2.reset();
+    assertThat(metric_map.get(metric1).get(), equalTo(0L));
+    cell_2.deleteIfZero();
+
+    assertThat(metric_map.get(metric1), is(nullValue()));
+
+    // Incrementing a deleted counter will recreate it.
+    cell_1.inc(20);
+    assertThat(metric_map.get(metric1).get(), equalTo(20L));
+  }
+
+  @Test
+  public void testRemoveSafeDeltaCounterCell_deleteNonZero() throws Exception {
+    ConcurrentHashMap<MetricName, AtomicLong> metric_map = new ConcurrentHashMap<>();
+    MetricName metric1 = MetricName.named("namespace", "name_1");
+
+    RemoveSafeDeltaCounterCell cell_1 = new RemoveSafeDeltaCounterCell(metric1, metric_map);
+    cell_1.inc(10);
+
+    cell_1.deleteIfZero();
+    assertThat(metric_map.size(), equalTo(1));
+    assertThat(metric_map.get(metric1).get(), equalTo(10L));
+  }
+
+  @Test
+  public void testRemoveSafeDeltaCounterCell_multipleUpdates() throws Exception {
+    ConcurrentHashMap<MetricName, AtomicLong> metric_map = new ConcurrentHashMap<>();
+    MetricName metric1 = MetricName.named("namespace", "name_1");
+
+    RemoveSafeDeltaCounterCell cell_1 = new RemoveSafeDeltaCounterCell(metric1, metric_map);
+
+    cell_1.inc();
+    assertThat(metric_map.get(metric1).get(), equalTo(1L));
+
+    cell_1.inc(5);
+    assertThat(metric_map.get(metric1).get(), equalTo(6L));
+
+    cell_1.dec(10);
+    assertThat(metric_map.get(metric1).get(), equalTo(-4L));
+
+    cell_1.dec();
+    assertThat(metric_map.get(metric1).get(), equalTo(-5L));
+  }
+
+  @Test
+  public void testRemoveSafeDeltaCounterCell_resetNonExistant() throws Exception {
+    ConcurrentHashMap<MetricName, AtomicLong> metric_map = new ConcurrentHashMap<>();
+    MetricName metric1 = MetricName.named("namespace", "name_1");
+
+    RemoveSafeDeltaCounterCell cell_1 = new RemoveSafeDeltaCounterCell(metric1, metric_map);
+    assertThat(metric_map.size(), equalTo(0));
+
+    cell_1.reset();
+    assertThat(metric_map.size(), equalTo(0));
+  }
+}