[GitHub] [incubator-druid] clintropolis commented on a change in pull request #7185: Avoid many unnecessary materializations of collections of 'all segments in cluster' cardinality

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

clintropolis commented on a change in pull request #7185: Avoid many unnecessary materializations of collections of 'all segments in cluster' cardinality
URL: https://github.com/apache/incubator-druid/pull/7185#discussion_r262817348
 
 

 ##########
 File path: server/src/main/java/org/apache/druid/server/coordinator/helper/DruidCoordinatorSegmentInfoLoader.java
 ##########
 @@ -42,8 +42,17 @@ public DruidCoordinatorRuntimeParams run(DruidCoordinatorRuntimeParams params)
   {
     log.info("Starting coordination. Getting available segments.");
 
-    // Display info about all available segments
-    final Set<DataSegment> availableSegments = coordinator.getOrderedAvailableDataSegments();
+    final TreeSet<DataSegment> availableSegments = DruidCoordinatorRuntimeParams.createAvailableSegmentsSet();
+    for (DataSegment segment : coordinator.iterateAvailableDataSegments()) {
+      if (segment.getSize() < 0) {
+        log.makeAlert("No size on a segment")
 
 Review comment:
   I know this isn't introduced here, but is it actually useful? This check looks like it has been here since the initial OSS commit, though in that form it looks like it attempted to do something if size was less than 0.
   
   It seems like this should happen, and we don't actually do anything other than alert if it does happen, so does it really qualify as an alert?
   
   The main reason I ask is it seems like the 3 places we are calling `DruidCoordinatorRuntimeParams.createAvailableSegmentsSet()` we then immediately fill it up afterwards, perhaps it could just take an `Iterable<DataSegment>` instead and could just produce the populated `TreeSet` directly if we didn't need this alert.

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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org