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/02/29 05:21:19 UTC
[GitHub] [druid] jihoonson opened a new pull request #9441: Improve
OvershadowableManager performance
jihoonson opened a new pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441
Fixes #9383.
### Description
Two key changes:
- Using the `Iterator` instead of `higherKey()` or `higherEntry()`.
- Using the `Iterator` API instead of `Stream`.
Here are the benchmark results.
master
| Benchmark | numInitialRootGenSegmentsPerInterval | numNonRootGenerations | segmentGranularity | useSegmentLock | Score | Score Error (99.9%) | Unit |
| -- | -- | -- | -- | -- | -- | -- | -- |
| org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | MONTH | FALSE | 74.593736 | 0.157821 | ops/s |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | MONTH | TRUE | 61.296126 | 0.289691 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | DAY | FALSE | 4.027792 | 0.050178 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | DAY | TRUE | 2.214782 | 0.009032 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | MONTH | FALSE | 69.753337 | 0.118109 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | MONTH | TRUE | 62.510767 | 0.249751 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | DAY | FALSE | 3.355433 | 0.013972 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | DAY | TRUE | 2.141556 | 0.00617 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | MONTH | FALSE | 3911.931702 | 14.962425 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | MONTH | TRUE | 187.36298 | 0.75218 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | DAY | FALSE | 94.90354 | 0.287476 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | DAY | TRUE | 5.258894 | 0.020194 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | MONTH | FALSE | 2366.365056 | 2.69362 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | MONTH | TRUE | 599.976444 | 1.165907 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | DAY | FALSE | 59.840876 | 0.178956 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | DAY | TRUE | 15.444807 | 0.038935 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | MONTH | FALSE | 320817.4244 | 934.954223 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | MONTH | TRUE | 476919.8161 | 1121.243296 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | DAY | FALSE | 263759.8788 | 3747.8076 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | DAY | TRUE | 326754.8762 | 7650.798385 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | MONTH | FALSE | 449372.3674 | 1293.973306 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | MONTH | TRUE | 577509.4054 | 1953.6536 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | DAY | FALSE | 281385.7145 | 11966.14959 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | DAY | TRUE | 389066.6441 | 6112.571051 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | MONTH | FALSE | 113573.3886 | 179.29859 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | MONTH | TRUE | 51644.26753 | 159.035054 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | DAY | FALSE | 71691.90114 | 3125.221368 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | DAY | TRUE | 23271.51767 | 902.091057 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | MONTH | FALSE | 111247.1351 | 174.299341 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | MONTH | TRUE | 36208.02057 | 48.240014 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | DAY | FALSE | 73012.95922 | 482.553345 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | DAY | TRUE | 15486.10912 | 976.58672 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | MONTH | FALSE | 91.150018 | 0.442367 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | MONTH | TRUE | 23.28416 | 0.177942 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | DAY | FALSE | 2.865762 | 0.0207 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | DAY | TRUE | 0.722529 | 0.007548 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | MONTH | FALSE | 99.904202 | 0.830639 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | MONTH | TRUE | 1.562742 | 0.089012 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | DAY | FALSE | 2.142417 | 0.017723 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | DAY | TRUE | 0.047155 | 0.001535 | ops/s | | | | | | | | | | | |
After PR
|Benchmark | Param: numInitialRootGenSegmentsPerInterval | Param: numNonRootGenerations | Param: segmentGranularity | Param: useSegmentLock | Score | Score Error (99.9%) | Unit |
| -- | -- | -- | -- | -- | -- | -- | -- |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | MONTH | FALSE | 423.598695 | 3.520175 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | MONTH | TRUE | 165.432408 | 1.179293 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | DAY | FALSE | 15.926139 | 0.051969 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 1 | DAY | TRUE | 4.787306 | 0.011304 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | MONTH | FALSE | 296.005812 | 2.672103 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | MONTH | TRUE | 150.081513 | 0.39904 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | DAY | FALSE | 12.446414 | 0.085265 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchAdd | 100 | 2 | DAY | TRUE | 4.223133 | 0.007413 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | MONTH | FALSE | 4348.161344 | 3.31101 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | MONTH | TRUE | 946.35651 | 4.968384 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | DAY | FALSE | 103.790997 | 0.711689 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 1 | DAY | TRUE | 25.720033 | 0.147587 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | MONTH | FALSE | 2638.004319 | 2.163262 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | MONTH | TRUE | 659.062763 | 2.468865 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | DAY | FALSE | 63.55069 | 0.170092 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchFindFullyOvershadowed | 100 | 2 | DAY | TRUE | 19.008214 | 0.135881 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | MONTH | FALSE | 705123.9855 | 3619.138328 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | MONTH | TRUE | 806885.8445 | 1038.68212 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | DAY | FALSE | 409478.6873 | 21179.31478 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 1 | DAY | TRUE | 486011.2014 | 22202.02013 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | MONTH | FALSE | 890916.9849 | 1583.87703 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | MONTH | TRUE | 1028235.656 | 4183.111624 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | DAY | FALSE | 460951.8017 | 17596.90437 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchIsOvershadowed | 100 | 2 | DAY | TRUE | 540136.4034 | 21796.82371 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | MONTH | FALSE | 135469.5003 | 367.690318 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | MONTH | TRUE | 57444.90682 | 109.662664 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | DAY | FALSE | 67781.1601 | 1669.652261 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 1 | DAY | TRUE | 19968.74589 | 1366.281118 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | MONTH | FALSE | 132912.5099 | 182.816225 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | MONTH | TRUE | 34443.96685 | 127.638394 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | DAY | FALSE | 66278.64465 | 2923.865498 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchLookup | 100 | 2 | DAY | TRUE | 15643.27084 | 449.80444 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | MONTH | FALSE | 479.409132 | 9.566923 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | MONTH | TRUE | 118.908165 | 0.309454 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | DAY | FALSE | 5.717129 | 0.034974 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 1 | DAY | TRUE | 2.408836 | 0.029314 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | MONTH | FALSE | 345.903707 | 1.457845 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | MONTH | TRUE | 107.075497 | 0.771716 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | DAY | FALSE | 3.769484 | 0.057692 | ops/s | | | | | | | | | | | |
org.apache.druid.timeline.VersionedIntervalTimelineBenchmark.benchRemove | 100 | 2 | DAY | TRUE | 1.986003 | 0.008282 | ops/s | | | | | | | | | | | |
<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.)
- [x] 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.
- [ ] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `OvershadowableManager`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r387208007
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -161,11 +162,11 @@ public int getNumObjects()
*/
public Set<ObjectType> findNonOvershadowedObjectsInInterval(Interval interval, Partitions completeness)
Review comment:
missing unit tests for this function?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389987936
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -509,12 +509,13 @@ private SegmentsToCompact findSegmentsToCompact(
private QueueEntry(List<DataSegment> segments)
{
Preconditions.checkArgument(segments != null && !segments.isEmpty());
- Collections.sort(segments);
+ final List<DataSegment> segmentsToSort = new ArrayList<>(segments);
+ Collections.sort(segmentsToSort);
this.interval = new Interval(
- segments.get(0).getInterval().getStart(),
- segments.get(segments.size() - 1).getInterval().getEnd()
+ segmentsToSort.get(0).getInterval().getStart(),
+ segmentsToSort.get(segmentsToSort.size() - 1).getInterval().getEnd()
);
- this.segments = segments;
+ this.segments = segmentsToSort;
Review comment:
👍 fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389945473
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java
##########
@@ -105,12 +105,46 @@
this.overshadowedGroups = new TreeMap<>();
}
- OvershadowableManager(OvershadowableManager<T> other)
+ public OvershadowableManager<T> copyVisible()
{
- this.knownPartitionChunks = new HashMap<>(other.knownPartitionChunks);
- this.standbyGroups = new TreeMap<>(other.standbyGroups);
- this.visibleGroupPerRange = new TreeMap<>(other.visibleGroupPerRange);
- this.overshadowedGroups = new TreeMap<>(other.overshadowedGroups);
+ final OvershadowableManager<T> copy = new OvershadowableManager<>();
+ visibleGroupPerRange.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.visibleGroupPerRange.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ return copy;
+ }
+
+ public OvershadowableManager<T> deepCopy()
+ {
+ final OvershadowableManager<T> copy = copyVisible();
+ overshadowedGroups.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.overshadowedGroups.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ standbyGroups.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.standbyGroups.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ return copy;
Review comment:
missing tests to verify that the returned copy is a `deepCopy()` - what if new fields are added to the overshadowable manager? Similar comments for `copyVisible()`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389855207
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -161,11 +162,11 @@ public int getNumObjects()
*/
public Set<ObjectType> findNonOvershadowedObjectsInInterval(Interval interval, Partitions completeness)
Review comment:
Added some.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441: Improve
OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389987875
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java
##########
@@ -105,12 +105,46 @@
this.overshadowedGroups = new TreeMap<>();
}
- OvershadowableManager(OvershadowableManager<T> other)
+ public OvershadowableManager<T> copyVisible()
{
- this.knownPartitionChunks = new HashMap<>(other.knownPartitionChunks);
- this.standbyGroups = new TreeMap<>(other.standbyGroups);
- this.visibleGroupPerRange = new TreeMap<>(other.visibleGroupPerRange);
- this.overshadowedGroups = new TreeMap<>(other.overshadowedGroups);
+ final OvershadowableManager<T> copy = new OvershadowableManager<>();
+ visibleGroupPerRange.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.visibleGroupPerRange.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ return copy;
+ }
+
+ public OvershadowableManager<T> deepCopy()
Review comment:
From the Item 13 (Override clone judiciously) of the book "Effective Java",
> Is all this complexity really necessary? Rarely. If you extend a class that already implements `Cloneable`, you have little choice but to implement a well-behaved `clone` method. Otherwise, you are usually better off providing an alternative means of object copying. **A better approach to object copying is to provide a _copy constructor_ or _copy factory_**.
I changed this method to a static factory based on this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r387246293
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -482,7 +483,12 @@ public boolean isOvershadowed(Interval interval, VersionType version, ObjectType
if (versionCompare > 0) {
return false;
} else if (versionCompare == 0) {
- if (timelineEntry.partitionHolder.stream().noneMatch(chunk -> chunk.getObject().overshadows(object))) {
+ // Intentionally use the Iterators API instead of the stream API for performance.
+ //noinspection ConstantConditions
+ final boolean nonOvershadowedObject = Iterators.all(
+ timelineEntry.partitionHolder.iterator(), chunk -> !chunk.getObject().overshadows(object)
Review comment:
Are we guaranteed that `chunk.getObject` will never be null?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389947980
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -509,12 +509,13 @@ private SegmentsToCompact findSegmentsToCompact(
private QueueEntry(List<DataSegment> segments)
{
Preconditions.checkArgument(segments != null && !segments.isEmpty());
- Collections.sort(segments);
+ final List<DataSegment> segmentsToSort = new ArrayList<>(segments);
+ Collections.sort(segmentsToSort);
this.interval = new Interval(
- segments.get(0).getInterval().getStart(),
- segments.get(segments.size() - 1).getInterval().getEnd()
+ segmentsToSort.get(0).getInterval().getStart(),
+ segmentsToSort.get(segmentsToSort.size() - 1).getInterval().getEnd()
);
- this.segments = segments;
+ this.segments = segmentsToSort;
Review comment:
Does the order of the segments matter?
Why not just calculate the min start and max end with O(n) implementation by looping over `segments`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389944874
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java
##########
@@ -105,12 +105,46 @@
this.overshadowedGroups = new TreeMap<>();
}
- OvershadowableManager(OvershadowableManager<T> other)
+ public OvershadowableManager<T> copyVisible()
{
- this.knownPartitionChunks = new HashMap<>(other.knownPartitionChunks);
- this.standbyGroups = new TreeMap<>(other.standbyGroups);
- this.visibleGroupPerRange = new TreeMap<>(other.visibleGroupPerRange);
- this.overshadowedGroups = new TreeMap<>(other.overshadowedGroups);
+ final OvershadowableManager<T> copy = new OvershadowableManager<>();
+ visibleGroupPerRange.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.visibleGroupPerRange.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ return copy;
+ }
+
+ public OvershadowableManager<T> deepCopy()
Review comment:
should this override `public OvershadowableManager<T> clone()` 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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389856246
##########
File path: core/src/main/java/org/apache/druid/timeline/VersionedIntervalTimeline.java
##########
@@ -482,7 +483,12 @@ public boolean isOvershadowed(Interval interval, VersionType version, ObjectType
if (versionCompare > 0) {
return false;
} else if (versionCompare == 0) {
- if (timelineEntry.partitionHolder.stream().noneMatch(chunk -> chunk.getObject().overshadows(object))) {
+ // Intentionally use the Iterators API instead of the stream API for performance.
+ //noinspection ConstantConditions
+ final boolean nonOvershadowedObject = Iterators.all(
+ timelineEntry.partitionHolder.iterator(), chunk -> !chunk.getObject().overshadows(object)
Review comment:
If you are asking whether we guarantee that programmatically, then I don't think so. But the object in the chunk should never be null.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r389987913
##########
File path: core/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java
##########
@@ -105,12 +105,46 @@
this.overshadowedGroups = new TreeMap<>();
}
- OvershadowableManager(OvershadowableManager<T> other)
+ public OvershadowableManager<T> copyVisible()
{
- this.knownPartitionChunks = new HashMap<>(other.knownPartitionChunks);
- this.standbyGroups = new TreeMap<>(other.standbyGroups);
- this.visibleGroupPerRange = new TreeMap<>(other.visibleGroupPerRange);
- this.overshadowedGroups = new TreeMap<>(other.overshadowedGroups);
+ final OvershadowableManager<T> copy = new OvershadowableManager<>();
+ visibleGroupPerRange.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.visibleGroupPerRange.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ return copy;
+ }
+
+ public OvershadowableManager<T> deepCopy()
+ {
+ final OvershadowableManager<T> copy = copyVisible();
+ overshadowedGroups.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.overshadowedGroups.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ standbyGroups.forEach((partitionRange, versionToGroups) -> {
+ // There should be only one group per partition range
+ final AtomicUpdateGroup<T> group = versionToGroups.values().iterator().next();
+ group.getChunks().forEach(chunk -> copy.knownPartitionChunks.put(chunk.getChunkNumber(), chunk));
+
+ copy.standbyGroups.put(
+ partitionRange,
+ new SingleEntryShort2ObjectSortedMap<>(group.getMinorVersion(), AtomicUpdateGroup.copy(group))
+ );
+ });
+ return copy;
Review comment:
Added 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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on issue #9441: Improve
OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on issue #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#issuecomment-596769079
I'm not sure when to override clone and when not to. I think it's meant to be used for deepCopies, so it seems to be a good fit for the function you implemented. Only change that probably matters is in NewestSegmentFirstIterator
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r390011128
##########
File path: core/src/test/java/org/apache/druid/timeline/partition/OvershadowableManagerTest.java
##########
@@ -63,6 +64,57 @@ public void setup()
expectedStandbyChunks = new ArrayList<>();
}
+ @Test
+ public void testCopyVisible()
+ {
+ // chunks of partition id 0 and 1
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // chunks to overshadow the partition id range [0, 2)
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+
+ // chunks of partition id 3 and 4
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // standby chunk
+ manager.addChunk(newNonRootChunk(2, 4, 1, 3));
+
+ OvershadowableManager<OvershadowableInteger> copy = OvershadowableManager.copyVisible(manager);
+ Assert.assertTrue(copy.getOvershadowedChunks().isEmpty());
+ Assert.assertTrue(copy.getStandbyChunks().isEmpty());
+ Assert.assertEquals(
+ Lists.newArrayList(manager.visibleChunksIterator()),
+ Lists.newArrayList(copy.visibleChunksIterator())
+ );
+ }
+
+ @Test
+ public void testDeepCopy()
+ {
+ // chunks of partition id 0 and 1
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // chunks to overshadow the partition id range [0, 2)
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+
+ // chunks of partition id 3 and 4
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // standby chunk
+ manager.addChunk(newNonRootChunk(2, 4, 1, 3));
+
+ OvershadowableManager<OvershadowableInteger> copy = OvershadowableManager.deepCopy(manager);
+ Assert.assertEquals(manager, copy);
+ }
+
Review comment:
Thanks for the tests! One last ask, since this depends on equals to be implemented correctly, I think we need another test for equals
```suggestion
@Test
public void testEqualAndHashCodeContract()
{
EqualsVerifier.forClass(OvershadowableManager.class).usingGetClass().verify();
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9441:
Improve OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#discussion_r390013562
##########
File path: core/src/test/java/org/apache/druid/timeline/partition/OvershadowableManagerTest.java
##########
@@ -63,6 +64,57 @@ public void setup()
expectedStandbyChunks = new ArrayList<>();
}
+ @Test
+ public void testCopyVisible()
+ {
+ // chunks of partition id 0 and 1
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // chunks to overshadow the partition id range [0, 2)
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+
+ // chunks of partition id 3 and 4
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // standby chunk
+ manager.addChunk(newNonRootChunk(2, 4, 1, 3));
+
+ OvershadowableManager<OvershadowableInteger> copy = OvershadowableManager.copyVisible(manager);
+ Assert.assertTrue(copy.getOvershadowedChunks().isEmpty());
+ Assert.assertTrue(copy.getStandbyChunks().isEmpty());
+ Assert.assertEquals(
+ Lists.newArrayList(manager.visibleChunksIterator()),
+ Lists.newArrayList(copy.visibleChunksIterator())
+ );
+ }
+
+ @Test
+ public void testDeepCopy()
+ {
+ // chunks of partition id 0 and 1
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // chunks to overshadow the partition id range [0, 2)
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+ manager.addChunk(newNonRootChunk(0, 2, 1, 3));
+
+ // chunks of partition id 3 and 4
+ manager.addChunk(newRootChunk());
+ manager.addChunk(newRootChunk());
+
+ // standby chunk
+ manager.addChunk(newNonRootChunk(2, 4, 1, 3));
+
+ OvershadowableManager<OvershadowableInteger> copy = OvershadowableManager.deepCopy(manager);
+ Assert.assertEquals(manager, copy);
+ }
+
Review comment:
Oops, added tests for OvershadowableManager and AtomicUpdateGroup.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s edited a comment on issue #9441: Improve
OvershadowableManager performance
Posted by GitBox <gi...@apache.org>.
suneet-s edited a comment on issue #9441: Improve OvershadowableManager performance
URL: https://github.com/apache/druid/pull/9441#issuecomment-596769079
I'm not sure when to override clone and when not to. I think it's meant to be used for deepCopies, so it seems to be a good fit for the function you implemented. Only change that probably matters is in NewestSegmentFirstIterator and tests for the deepCopy methods
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org