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 2020/07/02 11:01:16 UTC
[GitHub] [druid] jihoonson opened a new pull request #10125: Fix CachingClusteredClient when querying specific segments
jihoonson opened a new pull request #10125:
URL: https://github.com/apache/druid/pull/10125
### Description
`CachingClusteredClient.getQueryRunnerForSegments()` can be called when `RetryQueryRunner` retries the query for missing segments. Since it is possible to query only a subset of core partition set per time chunk, `CachingClusteredClient` should use `TimelineLookup.lookupWithIncompletePartitions()` instead. Maybe later, it is possible to refactor `CachingClusteredClient.getQueryRunnerForSegments()` to use the given segments as they are instead of creating a timeline with them.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid 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] gianm commented on a change in pull request #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449226894
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
Hmm, although, `getQueryRunnerForSegments` does create a modified timeline that only has the relevant segments, so this might be okay as long as we can assume none of them overshadow the others. Which should be a fair assumption for a query that originally started out as being by intervals. It seems a bit sketchy, though, and would be nice if we could avoid the unnecessary creation of this timeline.
But if we stick with this approach — will this properly retain information about the relevant SegmentDescriptors' intervals, though? These could be narrower than the segments' full intervals.
----------------------------------------------------------------
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 #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449312134
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
I think the test to verify this behavior should be an integration test rather than unit test. I will add some with doc/comments in a follow-up for integration 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] jihoonson commented on a change in pull request #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449254333
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
> Hmm, although, `getQueryRunnerForSegments` does create a modified timeline that only has the relevant segments, so this might be okay as long as we can assume none of them overshadow the others. Which should be a fair assumption for a query that originally started out as being by intervals. It seems a bit sketchy, though, and would be nice if we could avoid the unnecessary creation of this timeline.
I agree. That would be cleaner and even more efficient since we can avoid unnecessary timeline build and lookup on it. However, since we are about to release our next version, I would like to avoid such big refactoring for now. It will be more risky and potentially delay the release.
> But if we stick with this approach — will this properly retain information about the relevant SegmentDescriptors' intervals, though? These could be narrower than the segments' full intervals.
It does truncate the interval, but the query server can handle it. The query runner for specific segments in `ServerManager` or `AppenderatorImpl` uses `TimelineLookup.findEntry()` to find the `PartitionHolder` containing the segments to query. In this `findEntry()`, an entry matches if its interval contains the given interval.
```java
public @Nullable PartitionHolder<ObjectType> findEntry(Interval interval, VersionType version)
{
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();
}
}
}
return null;
}
finally {
lock.readLock().unlock();
}
}
```
----------------------------------------------------------------
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 merged pull request #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10125:
URL: https://github.com/apache/druid/pull/10125
----------------------------------------------------------------
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 a change in pull request #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449187355
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
This still isn't right. It will get all segments for the time chunk. We should use `timeline.findEntry` when the incoming QSS calls for specific segments, like ServerManager.getQueryRunnerForSegments does.
----------------------------------------------------------------
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 #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449254333
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
> Hmm, although, `getQueryRunnerForSegments` does create a modified timeline that only has the relevant segments, so this might be okay as long as we can assume none of them overshadow the others. Which should be a fair assumption for a query that originally started out as being by intervals. It seems a bit sketchy, though, and would be nice if we could avoid the unnecessary creation of this timeline.
I agree. That would be cleaner and even more efficient since we can avoid unnecessary timeline build and lookup on it. However, since we are about to release our next version, I would like to avoid such big refactoring for now though. It will be more risky and potentially delay the release.
> But if we stick with this approach — will this properly retain information about the relevant SegmentDescriptors' intervals, though? These could be narrower than the segments' full intervals.
It does truncate the interval, but the query server can handle it. The query runner for specific segments in `ServerManager` or `AppenderatorImpl` uses `TimelineLookup.findEntry()` to find the `PartitionHolder` containing the segments to query. In this `findEntry()`, an entry matches if its interval contains the given interval.
```java
public @Nullable PartitionHolder<ObjectType> findEntry(Interval interval, VersionType version)
{
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();
}
}
}
return null;
}
finally {
lock.readLock().unlock();
}
}
```
----------------------------------------------------------------
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 #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10125:
URL: https://github.com/apache/druid/pull/10125#issuecomment-653275798
@clintropolis @jon-wei @gianm thank you for the review!
----------------------------------------------------------------
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 #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449306169
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
Ah yeah, it does that now. I can add some comments and update tests to verify that behavior.
----------------------------------------------------------------
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 a change in pull request #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449305674
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
> It does truncate the interval, but the query server can handle it. The query runner for specific segments in `ServerManager` or `AppenderatorImpl` uses `TimelineLookup.findEntry()` to find the `PartitionHolder` containing the segments to query. In this `findEntry()`, an entry matches if its interval contains the given interval.
This isn't what I'm worried about — I'm worried that the SegmentDescriptor with the shorter interval might not get properly retained in the retry. Will it? If so, I think it's ok, if a bit sketchy. It would be good to have tests for it since it's so sketchy.
----------------------------------------------------------------
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 a change in pull request #10125: Fix CachingClusteredClient when querying specific segments
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10125:
URL: https://github.com/apache/druid/pull/10125#discussion_r449324545
##########
File path: server/src/main/java/org/apache/druid/client/CachingClusteredClient.java
##########
@@ -401,11 +407,16 @@ private ClusterQueryResult(Sequence<T> sequence, int numQueryServers)
}
}
- private Set<SegmentServerSelector> computeSegmentsToQuery(TimelineLookup<String, ServerSelector> timeline)
+ private Set<SegmentServerSelector> computeSegmentsToQuery(
+ TimelineLookup<String, ServerSelector> timeline,
+ boolean specificSegments
+ )
{
+ final java.util.function.Function<Interval, List<TimelineObjectHolder<String, ServerSelector>>> lookupFn
+ = specificSegments ? timeline::lookupWithIncompletePartitions : timeline::lookup;
final List<TimelineObjectHolder<String, ServerSelector>> serversLookup = toolChest.filterSegments(
query,
- intervals.stream().flatMap(i -> timeline.lookup(i).stream()).collect(Collectors.toList())
+ intervals.stream().flatMap(i -> lookupFn.apply(i).stream()).collect(Collectors.toList())
Review comment:
OK, thanks, if you make sure the integration tests cover this case then that works for me.
----------------------------------------------------------------
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