kfaraz commented on code in PR #15952:
URL: https://github.com/apache/druid/pull/15952#discussion_r1531581102
##########
processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java:
##########
@@ -450,6 +450,18 @@ private Map<Interval, Map<VersionType, TimelineEntry>>
computeOvershadowedPartit
return overshadowedPartitionsTimeline;
}
+ /**
+ * 0) Determine the TimelineEntry with the given interval
+ * 1) If the major versions are equal, the segment is overshadowed iff any
of the segments in the TimelineEntry overshadows it.
+ * 2) If the major versions are different, simply return true if the
TimelineEntry has a greater major version. False otherwise.
+ * 3) If there is no TimelineEntry with the same interval, find one which
contains the input interval.
+ * 4) If a TimelineEntry is found in step 3, repeat steps 1, 2 with it.
Return false otherwise.
+ *
+ * @param interval The interval of the overshadowable object (segment)
+ * @param version The version of the overshadowable object (segment)
+ * @param object The overshadowable object to be determined as overshadowed
or not
+ * @return true iff the object is overshadowed in this timeline
+ */
Review Comment:
This javadoc seems more like an implementation detail rather than the
overall behaviour of this method. I don't think this is needed.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java:
##########
@@ -1063,12 +1063,20 @@ public DataSegment map(int index, ResultSet r,
StatementContext ctx) throws SQLE
if (segments.isEmpty()) {
log.info("No segments found in the database!");
} else {
- log.info("Polled and found %,d segments in the database",
segments.size());
+ log.info("Polled and found %,d segments in the database.",
segments.size());
Review Comment:
```suggestion
log.info("Polled and found [%,d] segments in the database.",
segments.size());
```
##########
processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java:
##########
@@ -33,17 +33,24 @@
public class PartitionHolder<T extends Overshadowable<T>> implements
Iterable<PartitionChunk<T>>
{
private final OvershadowableManager<T> overshadowableManager;
+ private short maxMinorVersion;
Review Comment:
As discussed with @AmatyaAvadhanula offline, there are other cases where
`PartitionHolder.add` and `remove` methods are called from inside this class
itself, such as while performing a `copy`. So it is best to maintain the
`maxMinorVersion` here itself.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java:
##########
@@ -1063,12 +1063,20 @@ public DataSegment map(int index, ResultSet r,
StatementContext ctx) throws SQLE
if (segments.isEmpty()) {
log.info("No segments found in the database!");
} else {
- log.info("Polled and found %,d segments in the database",
segments.size());
+ log.info("Polled and found %,d segments in the database.",
segments.size());
}
+
+ log.debug("Building datasources snapshot from polled segments.");
+ final long snapshotCreationStartTime = System.currentTimeMillis();
dataSourcesSnapshot = DataSourcesSnapshot.fromUsedSegments(
Iterables.filter(segments, Objects::nonNull), // Filter corrupted
entries (see above in this method).
dataSourceProperties
);
+ final long snapshotCreationEndTime = System.currentTimeMillis();
Review Comment:
Instead of this, initialize a Druid `Stopwatch` at the start of this method.
And in every log, print the result of `.millisElapsed()` to get a clear idea of
the total time spent after performing each step.
##########
processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java:
##########
@@ -34,16 +34,26 @@ public class PartitionHolder<T extends Overshadowable<T>>
implements Iterable<Pa
{
private final OvershadowableManager<T> overshadowableManager;
+ // Maintains the maximum minor version across all the added segments.
Review Comment:
This comment should instead be a javadoc in the `getMinorVersion` method.
##########
processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java:
##########
@@ -815,6 +832,11 @@ public PartitionHolder<ObjectType> getPartitionHolder()
return partitionHolder;
}
+ public short getMaxMinorVersion()
Review Comment:
Does this need to be public?
--
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]
