kfaraz commented on code in PR #13967:
URL: https://github.com/apache/druid/pull/13967#discussion_r1170125387


##########
server/src/main/java/org/apache/druid/client/DataSourcesSnapshot.java:
##########
@@ -167,6 +326,112 @@ private List<DataSegment> determineOvershadowedSegments()
         }
       }
     }
-    return overshadowedSegments;
+    return ImmutableSet.copyOf(overshadowedSegments);
+  }
+
+  public static Set<SegmentWithOvershadowedStatus> 
getSegmentsWithOvershadowedStatus(
+      Collection<ImmutableDruidDataSource> segments,
+      Set<DataSegment> overshadowedSegments,
+      Map<String, Set<SegmentId>> handedOffState)
+  {
+
+    final Stream<DataSegment> usedSegments = segments
+        .stream()
+        .flatMap(t -> t.getSegments().stream());
+
+    return usedSegments
+        .map(segment -> new SegmentWithOvershadowedStatus(
+            segment,
+            overshadowedSegments.contains(segment),
+            getHandedOffStateForSegment(handedOffState, 
segment.getDataSource(), segment.getId())
+             )
+        )
+        .collect(Collectors.toSet());
+  }
+
+  private static boolean getHandedOffStateForSegment(
+      Map<String, Set<SegmentId>> handedOffState,
+      String dataSource, SegmentId segmentId
+  )
+  {
+    return handedOffState
+        .getOrDefault(dataSource, new HashSet<>())
+        .contains(segmentId);
+  }
+
+  private static 
CircularBuffer<ChangeRequestHistory.Holder<List<DataSegmentChange>>> 
computeChanges(
+      Set<SegmentWithOvershadowedStatus> oldSegments,
+      Set<SegmentWithOvershadowedStatus> currentSegments,
+      CircularBuffer<ChangeRequestHistory.Holder<List<DataSegmentChange>>> 
oldChanges
+  )
+  {
+    if (oldSegments.isEmpty()) {
+      return new CircularBuffer<>(CHANGES_QUEUE_MAX_SIZE);
+    }
+
+    // a segment is added to the change set, if following changes:
+    // segmentId
+    // overshadowed state
+    // handed off state
+
+    Map<SegmentId, SegmentWithOvershadowedStatus> oldSegmentsMap =
+        oldSegments
+            .stream()
+            .collect(Collectors.toMap(
+                segment -> segment.getDataSegment().getId(),
+                Function.identity()));
+
+    Map<SegmentId, SegmentWithOvershadowedStatus> currentSegmentsMap =
+        currentSegments
+            .stream()
+            .collect(Collectors.toMap(
+                segment -> segment.getDataSegment().getId(),
+                Function.identity()));
+
+    Set<SegmentWithOvershadowedStatus> segmentToBeRemoved = 
Sets.difference(oldSegments, currentSegments);

Review Comment:
   To simplify this, the difference should be taken between the key sets of the 
oldSegmentsMap and newSegmentsMap. Taking difference of the 
`SegmentWithOvershadowStatus` is a little confusing as you need to check for 
nulls repeatedly and the reader has to be aware of the equals/hashCode impl of 
`SegmentWithOvershadowStatus`.
   
   By taking diff of the key sets instead, you would have 3 cases:
   - segments present in new only (case of SEGMENT_ADDED)
   - segments present in old only (SEGMENT_REMOVED)
   - segments present in both (handoff or overshadow might have changed)



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


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to