You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/02/16 11:43:25 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
abhishekagarwal87 opened a new pull request #10892:
URL: https://github.com/apache/druid/pull/10892
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #10892:
URL: https://github.com/apache/druid/pull/10892
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r589204110
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -284,15 +286,15 @@ private void addAll(
@Override
@Nullable
- public PartitionHolder<ObjectType> findEntry(Interval interval, VersionType version)
+ public PartitionChunk<ObjectType> findChunk(Interval interval, VersionType version, int partitionNum)
{
lock.readLock().lock();
try {
for (Entry<Interval, TreeMap<VersionType, TimelineEntry>> entry : allTimelineEntries.entrySet()) {
if (entry.getKey().equals(interval) || entry.getKey().contains(interval)) {
TimelineEntry foundEntry = entry.getValue().get(version);
if (foundEntry != null) {
- return foundEntry.getPartitionHolder().asImmutable();
Review comment:
Looks like its not. will delete this method as well as the class ImmutablePartitionHolder.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r586959536
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -284,15 +286,15 @@ private void addAll(
@Override
@Nullable
- public PartitionHolder<ObjectType> findEntry(Interval interval, VersionType version)
+ public PartitionChunk<ObjectType> findChunk(Interval interval, VersionType version, int partitionNum)
{
lock.readLock().lock();
try {
for (Entry<Interval, TreeMap<VersionType, TimelineEntry>> entry : allTimelineEntries.entrySet()) {
if (entry.getKey().equals(interval) || entry.getKey().contains(interval)) {
TimelineEntry foundEntry = entry.getValue().get(version);
if (foundEntry != null) {
- return foundEntry.getPartitionHolder().asImmutable();
Review comment:
is the asImmutable method still needed?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm edited a comment on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
gianm edited a comment on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-779963284
The change looks like a good idea to me. How about:
- changing all call sites
- removing `findEntry` and the PartitionHolder `asImmutable` method
- adding a performance test that verifies that CachingClusteredClient is able to do its work in a reasonable amount of time even when there are some crazy number of segments in a time chunk, like 100,000? (That ought to be enough for anybody.) The "reasonable" amount of time might need to be somewhat high to prevent flaky tests from being an issue. Maybe 5–15 seconds. In reality, we'd want this to happen much faster than that, but we don't want to cause flaky tests.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r589211684
##########
File path: core/src/test/java/org/apache/druid/timeline/VersionedIntervalTimelineTestBase.java
##########
@@ -98,6 +98,16 @@ static void assertValues(
Assert.assertEquals(expected, actualSet);
}
+ static void assertSingleElementChunks(
+ PartitionChunk<OvershadowableInteger> expected,
+ PartitionChunk<OvershadowableInteger> actual
+ )
+ {
+ SingleElementPartitionChunk<OvershadowableInteger> expectedSingle = (SingleElementPartitionChunk<OvershadowableInteger>) expected;
Review comment:
Since it's unrelated to my change, I will leave this one for now as it is. Though I did mark `SingleElementPartitionChunk` deprecated.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r587171546
##########
File path: server/src/test/java/org/apache/druid/client/CachingClusteredClientPerfTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import org.apache.druid.client.cache.CacheConfig;
+import org.apache.druid.client.cache.CachePopulator;
+import org.apache.druid.client.cache.MapCache;
+import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy;
+import org.apache.druid.client.selector.QueryableDruidServer;
+import org.apache.druid.client.selector.RandomServerSelectorStrategy;
+import org.apache.druid.client.selector.ServerSelector;
+import org.apache.druid.guice.http.DruidHttpClientConfig;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.java.util.common.guava.TestSequence;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryPlus;
+import org.apache.druid.query.QueryRunner;
+import org.apache.druid.query.QueryToolChest;
+import org.apache.druid.query.QueryToolChestWarehouse;
+import org.apache.druid.query.SegmentDescriptor;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.context.ResponseContext;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.MultipleSpecificSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.TestHelper;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.apache.druid.server.QueryScheduler;
+import org.apache.druid.server.coordination.ServerManagerTest;
+import org.apache.druid.server.coordination.ServerType;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.timeline.partition.LinearShardSpec;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ForkJoinPool;
+
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Performance tests for {@link CachingClusteredClient} can be added here. There is one test for a scenario
Review comment:
What performance does this class test? Is it a scalability test? It would be nice to make it clear.
##########
File path: core/src/test/java/org/apache/druid/timeline/VersionedIntervalTimelineTestBase.java
##########
@@ -98,6 +98,16 @@ static void assertValues(
Assert.assertEquals(expected, actualSet);
}
+ static void assertSingleElementChunks(
+ PartitionChunk<OvershadowableInteger> expected,
+ PartitionChunk<OvershadowableInteger> actual
+ )
+ {
+ SingleElementPartitionChunk<OvershadowableInteger> expectedSingle = (SingleElementPartitionChunk<OvershadowableInteger>) expected;
Review comment:
`SingleElementPartitionChunk` is used only by deprecated things such as `NoneShardSpec` and `Tranquility`. We should deprecate `SingleElementPartitionChunk` as well and stop using it.
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -849,4 +851,37 @@ public int hashCode()
return Objects.hash(trueInterval, version, partitionHolder);
}
}
+
+ public static class PartitionChunkEntry<VersionType, ObjectType>
+ {
+ private final Interval interval;
Review comment:
Please add javadoc explaining what this interval is. There are two types of interval in timeline. See `LogicalSegment`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r586947887
##########
File path: server/src/test/java/org/apache/druid/client/CachingClusteredClientPerfTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import org.apache.druid.client.cache.CacheConfig;
+import org.apache.druid.client.cache.CachePopulator;
+import org.apache.druid.client.cache.MapCache;
+import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy;
+import org.apache.druid.client.selector.QueryableDruidServer;
+import org.apache.druid.client.selector.RandomServerSelectorStrategy;
+import org.apache.druid.client.selector.ServerSelector;
+import org.apache.druid.guice.http.DruidHttpClientConfig;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.java.util.common.guava.TestSequence;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryPlus;
+import org.apache.druid.query.QueryRunner;
+import org.apache.druid.query.QueryToolChest;
+import org.apache.druid.query.QueryToolChestWarehouse;
+import org.apache.druid.query.SegmentDescriptor;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.context.ResponseContext;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.MultipleSpecificSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.TestHelper;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.apache.druid.server.QueryScheduler;
+import org.apache.druid.server.coordination.ServerManagerTest;
+import org.apache.druid.server.coordination.ServerType;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.timeline.partition.LinearShardSpec;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ForkJoinPool;
+
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Performance tests for {@link CachingClusteredClient} can be added here. There is one test for a scenario
+ * where a single interval has large number of segments.
+ */
+public class CachingClusteredClientPerfTest
+{
+
+ @Test(timeout = 10_000)
+ public void testGetQueryRunnerForSegments_singleIntervalLargeSegments()
+ {
+ final int segmentCount = 30_000;
+ final Interval interval = Intervals.of("2021-02-13/2021-02-14");
+ final List<SegmentDescriptor> segmentDescriptors = new ArrayList<>(segmentCount);
+ final List<DataSegment> dataSegments = new ArrayList<>(segmentCount);
+ final VersionedIntervalTimeline<String, ServerSelector> timeline = new VersionedIntervalTimeline<>(Ordering.natural());
+ final DruidServer server = new DruidServer(
+ "server",
+ "localhost:9000",
+ null,
+ Long.MAX_VALUE,
+ ServerType.HISTORICAL,
+ DruidServer.DEFAULT_TIER,
+ DruidServer.DEFAULT_PRIORITY
+ );
+
+ for (int ii = 0; ii < segmentCount; ii++) {
+ segmentDescriptors.add(new SegmentDescriptor(interval, "1", ii));
Review comment:
For the purpose of perf testing, does it matter if the segments have:
- different interval
- different versions
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-780348831
> The change looks like a good idea to me. How about:
>
> * changing all call sites
> * removing `findEntry` and the PartitionHolder `asImmutable` method
> * adding a performance test that verifies that CachingClusteredClient is able to do its work in a reasonable amount of time even when there are some crazy number of segments in a time chunk, like 100,000? (That ought to be enough for anybody.) The "reasonable" amount of time might need to be somewhat high to prevent flaky tests from being an issue. Maybe 5–15 seconds. In reality, we'd want this to happen much faster than that, but we don't want to cause flaky tests.
Yes. I plan to remove all `findEntry` calls and fix tests accordingly. I will add the perf test as well. Thanks for the tip.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-780079768
@abhishekagarwal87 Another idea I had is in the `CachingClusteredClient.getQueryRunnerForSegments` we can cache the result returned from `timeline.findEntry(spec.getInterval(), spec.getVersion())`. We can maintains a map while iterating through all the input `specs` and cache returned `PartitionHolder<ServerSelector>` for the interval/version pair. This is useful when the `Iterable<SegmentDescriptor> specs` contains a lot of same interval/version pair. For example, if new there are a lot of segments per interval, the specs can contains pretty much all a single interval/version pair.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-780354748
> @abhishekagarwal87 Another idea I had is in the `CachingClusteredClient.getQueryRunnerForSegments` we can cache the result returned from `timeline.findEntry(spec.getInterval(), spec.getVersion())`. We can maintains a map while iterating through all the input `specs` and cache returned `PartitionHolder<ServerSelector>` for the interval/version pair. This is useful when the `Iterable<SegmentDescriptor> specs` contains a lot of same interval/version pair. For example, if new there are a lot of segments per interval, the specs can contains pretty much all a single interval/version pair.
yeah, I thought about that too. I didn't go ahead with that approach since that would require creating a new temporary state variable. For queries with many intervals, it could be that we end up creating many map entries with deep copies of `OvershawdoableManager`. what do you think?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r589206011
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -856,4 +845,50 @@ String computeResultLevelCachingEtag(
return strategy.computeCacheKey(query);
}
}
+
+ private static class TimelineConverter implements UnaryOperator<TimelineLookup<String, ServerSelector>>
+ {
+ private final Iterable<SegmentDescriptor> specs;
+
+ TimelineConverter(final Iterable<SegmentDescriptor> specs)
+ {
+ this.specs = specs;
+ }
+
+ @Override
+ public TimelineLookup<String, ServerSelector> apply(TimelineLookup<String, ServerSelector> timeline)
+ {
+ final VersionedIntervalTimeline<String, ServerSelector> timeline2 =
+ new VersionedIntervalTimeline<>(Ordering.natural());
+ Iterator<PartitionChunkEntry<String, ServerSelector>> unfilteredIterator =
+ Iterators.transform(specs.iterator(), spec -> toChunkEntry(timeline, spec));
+ Iterator<PartitionChunkEntry<String, ServerSelector>> iterator = Iterators.filter(
+ unfilteredIterator,
+ Objects::nonNull
+ );
+ // We add all the entries via batch add to avoid overhead of single add call. The call to add an entry to interval
Review comment:
I removed unnecessary details and just put that addAll is much more efficient than add.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r587177122
##########
File path: core/src/test/java/org/apache/druid/timeline/VersionedIntervalTimelineTestBase.java
##########
@@ -98,6 +98,16 @@ static void assertValues(
Assert.assertEquals(expected, actualSet);
}
+ static void assertSingleElementChunks(
+ PartitionChunk<OvershadowableInteger> expected,
+ PartitionChunk<OvershadowableInteger> actual
+ )
+ {
+ SingleElementPartitionChunk<OvershadowableInteger> expectedSingle = (SingleElementPartitionChunk<OvershadowableInteger>) expected;
Review comment:
`SingleElementPartitionChunk` is used only by deprecated things such as `NoneShardSpec` and `Tranquility`. We should deprecate `SingleElementPartitionChunk` as well and stop using it. You can use `NumberedPartitionChunk` instead.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r589203123
##########
File path: server/src/test/java/org/apache/druid/client/CachingClusteredClientPerfTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import org.apache.druid.client.cache.CacheConfig;
+import org.apache.druid.client.cache.CachePopulator;
+import org.apache.druid.client.cache.MapCache;
+import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy;
+import org.apache.druid.client.selector.QueryableDruidServer;
+import org.apache.druid.client.selector.RandomServerSelectorStrategy;
+import org.apache.druid.client.selector.ServerSelector;
+import org.apache.druid.guice.http.DruidHttpClientConfig;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.java.util.common.guava.TestSequence;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryPlus;
+import org.apache.druid.query.QueryRunner;
+import org.apache.druid.query.QueryToolChest;
+import org.apache.druid.query.QueryToolChestWarehouse;
+import org.apache.druid.query.SegmentDescriptor;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.context.ResponseContext;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.MultipleSpecificSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.TestHelper;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.apache.druid.server.QueryScheduler;
+import org.apache.druid.server.coordination.ServerManagerTest;
+import org.apache.druid.server.coordination.ServerType;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.timeline.partition.LinearShardSpec;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ForkJoinPool;
+
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Performance tests for {@link CachingClusteredClient} can be added here. There is one test for a scenario
+ * where a single interval has large number of segments.
+ */
+public class CachingClusteredClientPerfTest
+{
+
+ @Test(timeout = 10_000)
+ public void testGetQueryRunnerForSegments_singleIntervalLargeSegments()
Review comment:
It was under a second after the change. Before the change, it was more than a minute though I didn't let it run completely so it may have been very high.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-780023709
Another thing: I see that `asImmutable` doesn't just return an immutable copy, it also calls `OvershadowableManager.copyVisible` to potentially not return all of the chunks. So cutting out the call to `asImmutable` would also cut out the call to `copyVisible`. I'm not sure what consequences that might have. Have you analyzed it and determined that it's OK to cut out that call?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r589201063
##########
File path: server/src/test/java/org/apache/druid/client/CachingClusteredClientPerfTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import org.apache.druid.client.cache.CacheConfig;
+import org.apache.druid.client.cache.CachePopulator;
+import org.apache.druid.client.cache.MapCache;
+import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy;
+import org.apache.druid.client.selector.QueryableDruidServer;
+import org.apache.druid.client.selector.RandomServerSelectorStrategy;
+import org.apache.druid.client.selector.ServerSelector;
+import org.apache.druid.guice.http.DruidHttpClientConfig;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.java.util.common.guava.TestSequence;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryPlus;
+import org.apache.druid.query.QueryRunner;
+import org.apache.druid.query.QueryToolChest;
+import org.apache.druid.query.QueryToolChestWarehouse;
+import org.apache.druid.query.SegmentDescriptor;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.context.ResponseContext;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.MultipleSpecificSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.TestHelper;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.apache.druid.server.QueryScheduler;
+import org.apache.druid.server.coordination.ServerManagerTest;
+import org.apache.druid.server.coordination.ServerType;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.timeline.partition.LinearShardSpec;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ForkJoinPool;
+
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Performance tests for {@link CachingClusteredClient} can be added here. There is one test for a scenario
+ * where a single interval has large number of segments.
+ */
+public class CachingClusteredClientPerfTest
+{
+
+ @Test(timeout = 10_000)
+ public void testGetQueryRunnerForSegments_singleIntervalLargeSegments()
+ {
+ final int segmentCount = 30_000;
+ final Interval interval = Intervals.of("2021-02-13/2021-02-14");
+ final List<SegmentDescriptor> segmentDescriptors = new ArrayList<>(segmentCount);
+ final List<DataSegment> dataSegments = new ArrayList<>(segmentCount);
+ final VersionedIntervalTimeline<String, ServerSelector> timeline = new VersionedIntervalTimeline<>(Ordering.natural());
+ final DruidServer server = new DruidServer(
+ "server",
+ "localhost:9000",
+ null,
+ Long.MAX_VALUE,
+ ServerType.HISTORICAL,
+ DruidServer.DEFAULT_TIER,
+ DruidServer.DEFAULT_PRIORITY
+ );
+
+ for (int ii = 0; ii < segmentCount; ii++) {
+ segmentDescriptors.add(new SegmentDescriptor(interval, "1", ii));
Review comment:
The bottlenecks are in `PartitionHolder` when it has too many objects. If segments have different intervals and versions, the size of `PartitionHolder` will be much smaller and there is unlikely to be a perf issue.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-779963284
The change looks like a good idea to me. How about:
- changing all call sites
- removing `findEntry` and the PartitionHolder `asImmutable` method
- adding a performance test that verifies that CachingClusteredClient is able to do its work in a reasonable amount of time even when there are some crazy number of segments in a time chunk, like 100,000? (That ought to be enough for anybody.) The "reasonable" amount of time might need to be somewhat high to prevent flaky tests from being an issue. Maybe 15 seconds. In reality, we'd want this to happen much faster than 15 seconds, but we don't want to cause flaky tests.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r589206935
##########
File path: server/src/test/java/org/apache/druid/client/CachingClusteredClientPerfTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import org.apache.druid.client.cache.CacheConfig;
+import org.apache.druid.client.cache.CachePopulator;
+import org.apache.druid.client.cache.MapCache;
+import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy;
+import org.apache.druid.client.selector.QueryableDruidServer;
+import org.apache.druid.client.selector.RandomServerSelectorStrategy;
+import org.apache.druid.client.selector.ServerSelector;
+import org.apache.druid.guice.http.DruidHttpClientConfig;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.java.util.common.guava.TestSequence;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryPlus;
+import org.apache.druid.query.QueryRunner;
+import org.apache.druid.query.QueryToolChest;
+import org.apache.druid.query.QueryToolChestWarehouse;
+import org.apache.druid.query.SegmentDescriptor;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.context.ResponseContext;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.MultipleSpecificSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.TestHelper;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.apache.druid.server.QueryScheduler;
+import org.apache.druid.server.coordination.ServerManagerTest;
+import org.apache.druid.server.coordination.ServerType;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.timeline.partition.LinearShardSpec;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ForkJoinPool;
+
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Performance tests for {@link CachingClusteredClient} can be added here. There is one test for a scenario
Review comment:
any kind of perf test that doesn't require a real cluster.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r586956807
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -856,4 +845,50 @@ String computeResultLevelCachingEtag(
return strategy.computeCacheKey(query);
}
}
+
+ private static class TimelineConverter implements UnaryOperator<TimelineLookup<String, ServerSelector>>
+ {
+ private final Iterable<SegmentDescriptor> specs;
+
+ TimelineConverter(final Iterable<SegmentDescriptor> specs)
+ {
+ this.specs = specs;
+ }
+
+ @Override
+ public TimelineLookup<String, ServerSelector> apply(TimelineLookup<String, ServerSelector> timeline)
+ {
+ final VersionedIntervalTimeline<String, ServerSelector> timeline2 =
+ new VersionedIntervalTimeline<>(Ordering.natural());
+ Iterator<PartitionChunkEntry<String, ServerSelector>> unfilteredIterator =
+ Iterators.transform(specs.iterator(), spec -> toChunkEntry(timeline, spec));
+ Iterator<PartitionChunkEntry<String, ServerSelector>> iterator = Iterators.filter(
+ unfilteredIterator,
+ Objects::nonNull
+ );
+ // We add all the entries via batch add to avoid overhead of single add call. The call to add an entry to interval
Review comment:
I think the comment can be a little more clear in mentioning that by avoiding to call add n times and calling addAll once reduces O(n)*n to O(n)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r576778439
##########
File path: core/src/main/java/org/apache/druid/timeline/TimelineLookup.java
##########
@@ -52,4 +53,7 @@
List<TimelineObjectHolder<VersionType, ObjectType>> lookupWithIncompletePartitions(Interval interval);
@Nullable PartitionHolder<ObjectType> findEntry(Interval interval, VersionType version);
Review comment:
we can remove this method entirely and replace all usages with `findChunk`
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-793121071
LGTM
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-781577467
It would seem this change alone is not enough. Adding a chunk to timeline is also O(N*N) since it calls `PartitionHolder.isComplete` which is O(N) op.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #10892: Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10892:
URL: https://github.com/apache/druid/pull/10892#discussion_r586949245
##########
File path: server/src/test/java/org/apache/druid/client/CachingClusteredClientPerfTest.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Ordering;
+import org.apache.druid.client.cache.CacheConfig;
+import org.apache.druid.client.cache.CachePopulator;
+import org.apache.druid.client.cache.MapCache;
+import org.apache.druid.client.selector.HighestPriorityTierSelectorStrategy;
+import org.apache.druid.client.selector.QueryableDruidServer;
+import org.apache.druid.client.selector.RandomServerSelectorStrategy;
+import org.apache.druid.client.selector.ServerSelector;
+import org.apache.druid.guice.http.DruidHttpClientConfig;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.guava.Sequence;
+import org.apache.druid.java.util.common.guava.TestSequence;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.DruidProcessingConfig;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryPlus;
+import org.apache.druid.query.QueryRunner;
+import org.apache.druid.query.QueryToolChest;
+import org.apache.druid.query.QueryToolChestWarehouse;
+import org.apache.druid.query.SegmentDescriptor;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.context.ResponseContext;
+import org.apache.druid.query.filter.DimFilter;
+import org.apache.druid.query.spec.MultipleIntervalSegmentSpec;
+import org.apache.druid.query.spec.MultipleSpecificSegmentSpec;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+import org.apache.druid.segment.TestHelper;
+import org.apache.druid.segment.join.NoopJoinableFactory;
+import org.apache.druid.server.QueryScheduler;
+import org.apache.druid.server.coordination.ServerManagerTest;
+import org.apache.druid.server.coordination.ServerType;
+import org.apache.druid.timeline.DataSegment;
+import org.apache.druid.timeline.VersionedIntervalTimeline;
+import org.apache.druid.timeline.partition.LinearShardSpec;
+import org.joda.time.Interval;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ForkJoinPool;
+
+import static org.mockito.ArgumentMatchers.any;
+
+/**
+ * Performance tests for {@link CachingClusteredClient} can be added here. There is one test for a scenario
+ * where a single interval has large number of segments.
+ */
+public class CachingClusteredClientPerfTest
+{
+
+ @Test(timeout = 10_000)
+ public void testGetQueryRunnerForSegments_singleIntervalLargeSegments()
Review comment:
Do we have perf different before and after this change?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10892: [Draft] Avoid expensive findEntry call in segment metadata query
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10892:
URL: https://github.com/apache/druid/pull/10892#issuecomment-780352536
> Another thing: I see that `asImmutable` doesn't just return an immutable copy, it also calls `OvershadowableManager.copyVisible` to potentially not return all of the chunks. So cutting out the call to `asImmutable` would also cut out the call to `copyVisible`. I'm not sure what consequences that might have. Have you analyzed it and determined that it's OK to cut out that call?
Good catch. As far as I can tell, cutting down that call should not make a difference. `copyVisible` retains only visible chunks in `knownPartitionChunks`. However, in the `getChunk` call too, we are only returning a chunk if its visible. There shouldn't be a mismatch in behavior after the change.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org