kfaraz commented on code in PR #19138:
URL: https://github.com/apache/druid/pull/19138#discussion_r2922543595
##########
processing/src/main/java/org/apache/druid/java/util/common/guava/Comparators.java:
##########
@@ -119,6 +119,34 @@ public int compare(Interval lhs, Interval rhs)
}
};
+ private static final Comparator<Interval> INTERVAL_BY_START = new
Comparator<>()
Review Comment:
Can't we just reuse the existing comparators `INTERVAL_BY_START_THEN_END`
and `INTERVAL_BY_END_THEN_START`?
##########
processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java:
##########
@@ -74,28 +75,77 @@
public class VersionedIntervalTimeline<VersionType, ObjectType extends
Overshadowable<ObjectType>>
implements TimelineLookup<VersionType, ObjectType>
{
+ private static final Logger logger = new
Logger(VersionedIntervalTimeline.class);
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true);
// Below timelines stores only *visible* timelineEntries
// adjusted interval -> timelineEntry
- private final NavigableMap<Interval, TimelineEntry>
completePartitionsTimeline = new TreeMap<>(
- Comparators.intervalsByStartThenEnd()
- );
+ private final NavigableMap<Interval, TimelineEntry>
completePartitionsTimeline;
// IncompletePartitionsTimeline also includes completePartitionsTimeline
// adjusted interval -> timelineEntry
@VisibleForTesting
- final NavigableMap<Interval, TimelineEntry> incompletePartitionsTimeline =
new TreeMap<>(
- Comparators.intervalsByStartThenEnd()
- );
+ final NavigableMap<Interval, TimelineEntry> incompletePartitionsTimeline;
// true interval -> version -> timelineEntry
private final Map<Interval, TreeMap<VersionType, TimelineEntry>>
allTimelineEntries = new HashMap<>();
+ private final IntervalTree<TreeMap<VersionType, TimelineEntry>>
allTimeIntervals = new IntervalTree<>(Comparators.intervalsByStart(),
Comparators.intervalsByEnd());
private final AtomicInteger numObjects = new AtomicInteger();
private final Comparator<? super VersionType> versionComparator;
// Set this to true if the client needs to skip tombstones upon lookup (like
the broker)
private final boolean skipObjectsWithNoData;
+ // 0 Legacy functionality
+ // 1 Use IntervalTree only for basic segment timeline
+ // 2 Use IntervalTree for querying segments as well
+ private enum IntervalTreeMatchMode
+ {
+ NONE,
+ ENTRIES_ONLY(Capability.ENTRIES),
+ ALL(Capability.ENTRIES, Capability.QUERY);
+
+ private enum Capability
+ {
+ ENTRIES, QUERY
+ }
+
+ final Set<Capability> capabilities;
+
+ IntervalTreeMatchMode(Capability... capabilities)
+ {
+ this.capabilities = Set.of(capabilities);
+ }
+
+ public boolean isEnabled(Capability capability)
+ {
+ return capabilities.contains(capability);
+ }
+ }
+
+ private static IntervalTreeMatchMode intervalTreeMatchMode =
IntervalTreeMatchMode.NONE;
+
+ static {
+ String mode =
System.getProperty("experimental.timeline.intervalTreeMatchMode");
Review Comment:
- Please don't use system properties here directly. Instead pass a flag into
the constructor of `VersionedIntervalTimeline` indicating whether the improved
datastructures should be used or not.
- Property name should be more like
`druid.segment.timeline.fastIntervalSearch` with possible values `true` and
`false`. (I don't think we need 3 modes).
- The class creating the timeline should pass in the correct value for the
flag.
##########
processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java:
##########
@@ -74,28 +75,77 @@
public class VersionedIntervalTimeline<VersionType, ObjectType extends
Overshadowable<ObjectType>>
implements TimelineLookup<VersionType, ObjectType>
{
+ private static final Logger logger = new
Logger(VersionedIntervalTimeline.class);
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true);
// Below timelines stores only *visible* timelineEntries
// adjusted interval -> timelineEntry
- private final NavigableMap<Interval, TimelineEntry>
completePartitionsTimeline = new TreeMap<>(
- Comparators.intervalsByStartThenEnd()
- );
+ private final NavigableMap<Interval, TimelineEntry>
completePartitionsTimeline;
// IncompletePartitionsTimeline also includes completePartitionsTimeline
// adjusted interval -> timelineEntry
@VisibleForTesting
- final NavigableMap<Interval, TimelineEntry> incompletePartitionsTimeline =
new TreeMap<>(
- Comparators.intervalsByStartThenEnd()
- );
+ final NavigableMap<Interval, TimelineEntry> incompletePartitionsTimeline;
// true interval -> version -> timelineEntry
private final Map<Interval, TreeMap<VersionType, TimelineEntry>>
allTimelineEntries = new HashMap<>();
+ private final IntervalTree<TreeMap<VersionType, TimelineEntry>>
allTimeIntervals = new IntervalTree<>(Comparators.intervalsByStart(),
Comparators.intervalsByEnd());
private final AtomicInteger numObjects = new AtomicInteger();
private final Comparator<? super VersionType> versionComparator;
// Set this to true if the client needs to skip tombstones upon lookup (like
the broker)
private final boolean skipObjectsWithNoData;
+ // 0 Legacy functionality
+ // 1 Use IntervalTree only for basic segment timeline
+ // 2 Use IntervalTree for querying segments as well
+ private enum IntervalTreeMatchMode
+ {
+ NONE,
+ ENTRIES_ONLY(Capability.ENTRIES),
Review Comment:
Is there a specific case when we would want to support improved search on
entries only and not queries?
I think just 2 modes should suffice: ALL or NONE
##########
processing/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java:
##########
@@ -74,28 +75,77 @@
public class VersionedIntervalTimeline<VersionType, ObjectType extends
Overshadowable<ObjectType>>
implements TimelineLookup<VersionType, ObjectType>
{
+ private static final Logger logger = new
Logger(VersionedIntervalTimeline.class);
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(true);
// Below timelines stores only *visible* timelineEntries
// adjusted interval -> timelineEntry
- private final NavigableMap<Interval, TimelineEntry>
completePartitionsTimeline = new TreeMap<>(
- Comparators.intervalsByStartThenEnd()
- );
+ private final NavigableMap<Interval, TimelineEntry>
completePartitionsTimeline;
// IncompletePartitionsTimeline also includes completePartitionsTimeline
// adjusted interval -> timelineEntry
@VisibleForTesting
- final NavigableMap<Interval, TimelineEntry> incompletePartitionsTimeline =
new TreeMap<>(
- Comparators.intervalsByStartThenEnd()
- );
+ final NavigableMap<Interval, TimelineEntry> incompletePartitionsTimeline;
// true interval -> version -> timelineEntry
private final Map<Interval, TreeMap<VersionType, TimelineEntry>>
allTimelineEntries = new HashMap<>();
+ private final IntervalTree<TreeMap<VersionType, TimelineEntry>>
allTimeIntervals = new IntervalTree<>(Comparators.intervalsByStart(),
Comparators.intervalsByEnd());
private final AtomicInteger numObjects = new AtomicInteger();
private final Comparator<? super VersionType> versionComparator;
// Set this to true if the client needs to skip tombstones upon lookup (like
the broker)
private final boolean skipObjectsWithNoData;
+ // 0 Legacy functionality
+ // 1 Use IntervalTree only for basic segment timeline
+ // 2 Use IntervalTree for querying segments as well
+ private enum IntervalTreeMatchMode
+ {
+ NONE,
+ ENTRIES_ONLY(Capability.ENTRIES),
+ ALL(Capability.ENTRIES, Capability.QUERY);
+
+ private enum Capability
+ {
+ ENTRIES, QUERY
+ }
+
+ final Set<Capability> capabilities;
+
+ IntervalTreeMatchMode(Capability... capabilities)
+ {
+ this.capabilities = Set.of(capabilities);
+ }
+
+ public boolean isEnabled(Capability capability)
+ {
+ return capabilities.contains(capability);
+ }
+ }
+
+ private static IntervalTreeMatchMode intervalTreeMatchMode =
IntervalTreeMatchMode.NONE;
+
+ static {
+ String mode =
System.getProperty("experimental.timeline.intervalTreeMatchMode");
+ if (mode != null) {
+ try {
+ intervalTreeMatchMode = IntervalTreeMatchMode.valueOf(mode);
+ }
+ catch (IllegalArgumentException e) {
+ logger.warn(e, "Unrecognized interval tree match mode specified [%s]",
mode);
+ }
+ }
+ }
+
+ {
Review Comment:
Please move this field initialization inside the constructor.
--
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]
