[GitHub] [druid] cryptoe commented on a diff in pull request #14403: Add replication factor column to sys table

["cryptoe (via GitHub)" <gi...@apache.org>]

cryptoe commented on code in PR #14403:
URL: https://github.com/apache/druid/pull/14403#discussion_r1230585260


##########
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java:
##########
@@ -97,6 +101,9 @@ public MetadataSegmentView(
     this.isCacheEnabled = config.isMetadataSegmentCacheEnable();
     this.pollPeriodInMS = config.getMetadataSegmentPollPeriod();
     this.scheduledExec = Execs.scheduledSingleThreaded("MetadataSegmentView-Cache--%d");
+    this.segmentIdToReplicationFactor = CacheBuilder.newBuilder()
+                                                    .expireAfterWrite(3, TimeUnit.MINUTES)

Review Comment:
   This can a more actually. Maybe 10 minutes?  And should it be expireAfterRead()?



##########
server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java:
##########
@@ -79,6 +80,7 @@ public static SegmentReplicantLookup make(DruidCluster cluster, boolean replicat
 
   private final Table<SegmentId, String, Integer> segmentsInCluster;
   private final Table<SegmentId, String, Integer> loadingSegments;
+  private final Map<SegmentId, Integer> segmentIdToReplicationFactor = new HashMap<>();

Review Comment:
   Who clears this Map ? Is it generated every coordinator duty run. 
   
   if yes then we should think of having an `int` as the value for performance reasons. 



##########
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java:
##########
@@ -77,6 +80,7 @@
    */
   @MonotonicNonNull
   private volatile ImmutableSortedSet<SegmentStatusInCluster> publishedSegments = null;
+  private final Cache<SegmentId, Integer> segmentIdToReplicationFactor;

Review Comment:
   Please add the reasoning for this cache as well ie flapping coordinators. 



##########
sql/src/main/java/org/apache/druid/sql/calcite/schema/MetadataSegmentView.java:
##########
@@ -143,10 +150,16 @@ private void poll()
     while (metadataSegments.hasNext()) {
       final SegmentStatusInCluster segment = metadataSegments.next();
       final DataSegment interned = DataSegmentInterner.intern(segment.getDataSegment());
+      Integer replicationFactor = segment.getReplicationFactor();
+      if (segment.getReplicationFactor() == null) {
+        replicationFactor = segmentIdToReplicationFactor.getIfPresent(segment.getDataSegment().getId());
+      } else {
+        segmentIdToReplicationFactor.put(segment.getDataSegment().getId(), segment.getReplicationFactor());
+      }
       final SegmentStatusInCluster segmentStatusInCluster = new SegmentStatusInCluster(
           interned,
           segment.isOvershadowed(),
-          segment.getReplicationFactor()
+          replicationFactor

Review Comment:
   replicationFactor still can be null rite ?



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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