[GitHub] [ozone] jojochuang commented on a change in pull request #2367: HDDS-5384 OM refreshPipeline should not invoke the expensive OmKeyLocationInfoGroup.getLocationList()

[GitBox <gi...@apache.org>]

jojochuang commented on a change in pull request #2367:
URL: https://github.com/apache/ozone/pull/2367#discussion_r658414430



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyLocationInfoGroup.java
##########
@@ -89,17 +90,27 @@ public long getVersion() {
     return version;
   }
 
+  /**
+   * Use this expensive method only when absolutely needed!
+   * It creates a new list so it is not an O(1) operation.
+   * Use getLocationLists() instead.
+   * @return a list of OmKeyLocationInfo
+   */
   public List<OmKeyLocationInfo> getLocationList() {
     return locationVersionMap.values().stream().flatMap(List::stream)
         .collect(Collectors.toList());
   }
 
+  public Collection<List<OmKeyLocationInfo>> getLocationLists() {
+    return locationVersionMap.values();
+  }
+
   public long getLocationListCount() {
     return locationVersionMap.values().stream().mapToLong(List::size).sum();
   }
 
-  public List<OmKeyLocationInfo> getLocationList(Long versionToFetch) {
-    return new ArrayList<>(locationVersionMap.get(versionToFetch));
+  public Collection<OmKeyLocationInfo> getLocationList(Long versionToFetch) {

Review comment:
       this is a minor issue but allocating an ArrayList is an O(n) operation. I don't see this one coming up in flamegraph (which means it's probably minimal) but it's a good practice to reduce it to O(1)




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org