pabloem commented on a change in pull request #14490:
URL: https://github.com/apache/beam/pull/14490#discussion_r622594151



##########
File path: 
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/ShortIdMap.java
##########
@@ -17,29 +17,46 @@
  */
 package org.apache.beam.runners.core.metrics;
 
+import java.util.HashMap;
+import java.util.Map;
 import java.util.NoSuchElementException;
 import org.apache.beam.model.pipeline.v1.MetricsApi.MonitoringInfo;
 import 
org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Preconditions;
-import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.collect.BiMap;
-import 
org.apache.beam.vendor.guava.v26_0_jre.com.google.common.collect.HashBiMap;
 
 /** A Class for registering short ids for MonitoringInfos. */
 public class ShortIdMap {
   private int counter = 0;
-  private BiMap<String, MonitoringInfo> monitoringInfoMap = HashBiMap.create();
+  // We must use two maps instead of a BiMap because the two maps store 
different MonitoringInfos.
+  // The values in shortIdToInfo contain the MonitoringInfo with only the 
payload cleared.
+  private Map<String, MonitoringInfo> shortIdToInfo = new HashMap<>();
+  // The keys in cleanedInfoToShortId contain a cleaned MonitoringInfo after 
clearing
+  // the payload, startTime and type.
+  private Map<MonitoringInfo, String> cleanedInfoToShortId = new HashMap<>();
 
   public synchronized String getOrCreateShortId(MonitoringInfo info) {
     Preconditions.checkNotNull(info);
-    String shortId = monitoringInfoMap.inverse().get(info);
+    // Remove the payload, startTime and typeUrn before using the 
MonitoringInfo as a key.
+    // As only the URN+labels uniquely identify the MonitoringInfo
+    MonitoringInfo.Builder cleaner = MonitoringInfo.newBuilder(info);
+    cleaner.clearPayload();
+    cleaner.clearStartTime();
+    cleaner.clearType();

Review comment:
       isn't the type also necessary to identify a monitoring info? What 
identifies it / what goes into the hash? Is it hash consistent?




-- 
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to