{"action":"created","issue":{"url":"https://api.github.com/repos/apache/incubator-druid/issues/7547","repository_url":"https://api.github.com/repos/apache/incubator-druid","labels_url":"https://api.github.com/repos/apache/incubator-druid/issues/7547/labels{/name}","comments_url":"https://api.github.com/repos/apache/incubator-druid/issues/7547/comments","events_url":"https://api.github.com/repos/apache/incubator-druid/issues/7547/events","html_url":"https://github.com/apache/incubator-druid/pull/7547","id":436964287,"node_id":"MDExOlB1bGxSZXF1ZXN0MjczMzQ0MDky","number":7547,"title":"Add support minor compaction with segment locking","user":{"login":"jihoonson","id":2322288,"node_id":"MDQ6VXNlcjIzMjIyODg=","avatar_url":"https://avatars3.githubusercontent.com/u/2322288?v=4","gravatar_id":"","url":"https://api.github.com/users/jihoonson","html_url":"https://github.com/jihoonson","followers_url":"https://api.github.com/users/jihoonson/followers","following_url":"https://api.github.com/users/jihoonson/following{/other_user}","gists_url":"https://api.github.com/users/jihoonson/gists{/gist_id}","starred_url":"https://api.github.com/users/jihoonson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jihoonson/subscriptions","organizations_url":"https://api.github.com/users/jihoonson/orgs","repos_url":"https://api.github.com/users/jihoonson/repos","events_url":"https://api.github.com/users/jihoonson/events{/privacy}","received_events_url":"https://api.github.com/users/jihoonson/received_events","type":"User","site_admin":false},"labels":[{"id":219582906,"node_id":"MDU6TGFiZWwyMTk1ODI5MDY=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Area%20-%20Batch%20Indexing","name":"Area - Batch Indexing","color":"fef2c0","default":false},{"id":690563043,"node_id":"MDU6TGFiZWw2OTA1NjMwNDM=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Area%20-%20Kafka/Kinesis%20Indexing","name":"Area - Kafka/Kinesis Indexing","color":"fef2c0","default":false},{"id":283617667,"node_id":"MDU6TGFiZWwyODM2MTc2Njc=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Incompatible","name":"Incompatible","color":"5319e7","default":false},{"id":297508895,"node_id":"MDU6TGFiZWwyOTc1MDg4OTU=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Release%20Notes","name":"Release Notes","color":"006b75","default":false}],"state":"open","locked":false,"assignee":{"login":"jon-wei","id":8729063,"node_id":"MDQ6VXNlcjg3MjkwNjM=","avatar_url":"https://avatars3.githubusercontent.com/u/8729063?v=4","gravatar_id":"","url":"https://api.github.com/users/jon-wei","html_url":"https://github.com/jon-wei","followers_url":"https://api.github.com/users/jon-wei/followers","following_url":"https://api.github.com/users/jon-wei/following{/other_user}","gists_url":"https://api.github.com/users/jon-wei/gists{/gist_id}","starred_url":"https://api.github.com/users/jon-wei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jon-wei/subscriptions","organizations_url":"https://api.github.com/users/jon-wei/orgs","repos_url":"https://api.github.com/users/jon-wei/repos","events_url":"https://api.github.com/users/jon-wei/events{/privacy}","received_events_url":"https://api.github.com/users/jon-wei/received_events","type":"User","site_admin":false},"assignees":[{"login":"jon-wei","id":8729063,"node_id":"MDQ6VXNlcjg3MjkwNjM=","avatar_url":"https://avatars3.githubusercontent.com/u/8729063?v=4","gravatar_id":"","url":"https://api.github.com/users/jon-wei","html_url":"https://github.com/jon-wei","followers_url":"https://api.github.com/users/jon-wei/followers","following_url":"https://api.github.com/users/jon-wei/following{/other_user}","gists_url":"https://api.github.com/users/jon-wei/gists{/gist_id}","starred_url":"https://api.github.com/users/jon-wei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jon-wei/subscriptions","organizations_url":"https://api.github.com/users/jon-wei/orgs","repos_url":"https://api.github.com/users/jon-wei/repos","events_url":"https://api.github.com/users/jon-wei/events{/privacy}","received_events_url":"https://api.github.com/users/jon-wei/received_events","type":"User","site_admin":false}],"milestone":null,"comments":2,"created_at":"2019-04-25T00:49:08Z","updated_at":"2019-07-16T20:08:51Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/apache/incubator-druid/pulls/7547","html_url":"https://github.com/apache/incubator-druid/pull/7547","diff_url":"https://github.com/apache/incubator-druid/pull/7547.diff","patch_url":"https://github.com/apache/incubator-druid/pull/7547.patch"},"body":"Apologize for the huge PR. I tried to split into sub PRs, but I think it's not easy split more because new changes for segment locking are tightly coupled together and it won't be easy to understand if they are in separate PRs. \r\n\r\nThis PR fixes https://github.com/apache/incubator-druid/issues/7491 and now it's ready for review.\r\n\r\nHere are the key classes to be reviewed.\r\n\r\n### Overshadowable\r\n\r\nNew interface to represent a class which can have overshadow relation between its instances. Ex) dataSegment\r\n\r\n### ShardSpecFactory\r\n\r\nNew interface to be used to allocate segments remotely in the overlord.\r\n\r\n### NumberedOverwriteShardSpec\r\n\r\nNew shardSpec for segments which may overshadow others with their minor version.\r\n\r\n### DataSegment\r\n\r\n- `getVersion` is renamed to `getMajorVersion`.\r\n- Some short circuit methods to check overshadow relation between segments.\r\n\r\n### VersionedIntervalTimeline\r\n\r\nImproved to consider the new overshadow relation properly.\r\n\r\n### OvershadowableManager\r\n\r\nA new class manages the state of AtomicUpdateGroup.\r\n\r\n### AtomicUpdateGruop\r\n\r\nA set of PartitionChunks which should be atomically visible or not in the timeline.\r\n\r\n### SegmentTransactionalInsertAction\r\n\r\nAdded a new parameter `segmentsToBeOverwritten`. Shouldn't be empty if the new segments are created with segment locks.\r\n\r\n### AbstractBatchIndexTask\r\n\r\nAbstract class for batch tasks like indexTask or parallelIndexTask. Provides some methods for easily acquiring task locks.\r\n\r\n### IndexTaskSegmentAllocator\r\n\r\nSegment allocator interface for only IndexTask. It has 3 different modes for allocating segments.\r\n\r\nThe new segment locking is currently available for only Kafka/Kinesis index task, index task, and parallel index task.\r\n\r\nThis PR is marked as `Incompatible` because it introduces a new shardSpec. It's not possible to roll back after this PR. For rolling update while kafka/kinesis indexing service is running, any kind of overwriting tasks (including compaction task and index task) should be stopped first. Otherwise, kafka/kinesis index tasks might fail when they list segments with the new shardSpec generated by overwriting tasks. Native parallel index tasks cannot be run in middleManagers of mixed versions, so they should be stopped first before update.\r\n\r\nCurrently there is a known issue of potential race in task locking. If two or more overwriting tasks try to acquire segment locks for overlapped sets of segments, each task acquires locks one by one per segment in the current implementation. This may lead to failures of both tasks if they hold locks necessary for each other. However, I think this happens rarely and so will fix in a follow-up PR.\r\n\r\nI've been testing this PR in our in-house cluster since I raised this PR and it looks fine so far."},"comment":{"url":"https://api.github.com/repos/apache/incubator-druid/issues/comments/511966275","html_url":"https://github.com/apache/incubator-druid/pull/7547#issuecomment-511966275","issue_url":"https://api.github.com/repos/apache/incubator-druid/issues/7547","id":511966275,"node_id":"MDEyOklzc3VlQ29tbWVudDUxMTk2NjI3NQ==","user":{"login":"jihoonson","id":2322288,"node_id":"MDQ6VXNlcjIzMjIyODg=","avatar_url":"https://avatars3.githubusercontent.com/u/2322288?v=4","gravatar_id":"","url":"https://api.github.com/users/jihoonson","html_url":"https://github.com/jihoonson","followers_url":"https://api.github.com/users/jihoonson/followers","following_url":"https://api.github.com/users/jihoonson/following{/other_user}","gists_url":"https://api.github.com/users/jihoonson/gists{/gist_id}","starred_url":"https://api.github.com/users/jihoonson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jihoonson/subscriptions","organizations_url":"https://api.github.com/users/jihoonson/orgs","repos_url":"https://api.github.com/users/jihoonson/repos","events_url":"https://api.github.com/users/jihoonson/events{/privacy}","received_events_url":"https://api.github.com/users/jihoonson/received_events","type":"User","site_admin":false},"created_at":"2019-07-16T20:08:51Z","updated_at":"2019-07-16T20:08:51Z","author_association":"CONTRIBUTOR","body":"@clintropolis I ran some benchmark and here are some results. The benchmark code is available in [my branch](https://github.com/jihoonson/druid/blob/minor-compaction-benchmark/benchmarks/src/main/java/org/apache/druid/timeline/VersionedIntervalTimelineBenchmark.java) and will raise another pr for it after this PR is merged. Please note that this benchmark doesn't compare the performance of `VersionedIntervalTimeline` of this PR with that of the master branch. The primary purpose of this benchmark is to measure how slow `VersionedIntervalTimeline` is with segmentLock after this PR. (When timeChunk lock is used, `VersionedIntervalTimeline` performs a bit more operations such as creating an `OvershadowableManager` of a single partitionChunk than that of master. However, the overall performance wouldn't be much different.)\r\n\r\nTL;DR `VersionedIntervalTimeline` is slower with segment lock than that with timeChunk lock, but it's still acceptable. \r\n\r\n### Data setup\r\n\r\nA synthetic segments were created to emulate the usual compaction scenario where initial segments are created and then they got compacted while new segments are appended. The benchmark first generates `numInitialRootGenSegmentsPerInterval` segments per interval. Then, it generates `numInitialRootGenSegmentsPerInterval * COMPACTED_SEGMENTS_RATIO_TO_INITIAL_SEGMENTS` compacted segments which overwrites the segments of the previous generation. It also generates new appending segments. This can be repeated more than once based on `numNonRootGenerations`.\r\n\r\nHere, I pasted only some of results, e.g., when segmentGranularity = MONTH or # of non-root generations = 5, because they look similar. In every graph, blue and red represent throughput when using timeChunk lock and segment lock, respectively. Line charts show the throughput with varying # of total segments (including overshadowed ones). The _log scale_ is used for y axis of all line charts. The bar charts just show the one data point in line charts where # of segments = 160 which is the number Druid dataSources usually have per timeChunk.\r\n\r\n#### Lookup\r\n\r\n![Lookup - varying # of segments](https://user-images.githubusercontent.com/2322288/61318527-28191980-a7ba-11e9-9a44-1028779af121.png)\r\n![Lookup - 160 segments](https://user-images.githubusercontent.com/2322288/61318529-294a4680-a7ba-11e9-8998-339754399671.png)\r\n\r\nTo measure the throughput of the `lookup` method, the benchmark picks up a random interval which spans 3 time chunks based on segmentGranularity. This result says the timeline is pretty faster with timeChunk lock than using segment lock. But 23620.913 ops/s is still fast. Regarding peak CPU usage, there's no big difference between using timeChunk lock and segment lock.\r\n\r\n#### FindFullyOvershadowed\r\n\r\n![findFullyOvershadowed - varying # of segments](https://user-images.githubusercontent.com/2322288/61318539-2d766400-a7ba-11e9-8db2-656d00ec4fe3.png)\r\n![findFullyOvershadowed - 160 segments](https://user-images.githubusercontent.com/2322288/61318543-2ea79100-a7ba-11e9-858a-58645de94d33.png)\r\n\r\n`findFullyOvershadowed` is very slow with segmentLock. This is because the implementation is not very optimized, especially the constructor of `PartitionHolder`. Here is the code snippet.\r\n\r\n```java\r\n  public PartitionHolder(List<PartitionChunk<T>> initialChunks)\r\n  {\r\n    this.overshadowableManager = new OvershadowableManager<>();\r\n    for (PartitionChunk<T> chunk : initialChunks) {\r\n      add(chunk);\r\n    }\r\n  }\r\n```\r\n\r\nThis could be improved by adding a bulk construction of OvershadowableManager, but `findFullyOvershadowed` is called only one place in production code, i.e., `ParallelSubIndexTask`. I think it could be done later rather than in this PR.\r\n\r\n#### IsOvershadowed\r\n\r\n![isOvershadowed - varying # of segments](https://user-images.githubusercontent.com/2322288/61318549-31a28180-a7ba-11e9-98fc-f6b7da33fcaa.png)\r\n![isOvershadowed - 160 segments](https://user-images.githubusercontent.com/2322288/61318552-32d3ae80-a7ba-11e9-88da-1efeee4bd879.png)\r\n\r\n`isOvershadowed` shows a pretty similar throughput. The benchmark chooses a random one among timeChunks populated based on segmentGranularity. As a result, the benchmark always executes the below part in `isOvershadowed` method.\r\n\r\n```java\r\n      TimelineEntry entry = completePartitionsTimeline.get(interval);\r\n      if (entry != null) {\r\n        final int majorVersionCompare = versionComparator.compare(version, entry.getVersion());\r\n        if (majorVersionCompare == 0) {\r\n          for (PartitionChunk<ObjectType> chunk : entry.partitionHolder) {\r\n            if (chunk.getObject().isOvershadow(object)) {\r\n              return true;\r\n            }\r\n          }\r\n          return false;\r\n        } else {\r\n          return majorVersionCompare < 0;\r\n        }\r\n      }\r\n```\r\n\r\nSince the number of visible segments to be iterated in the for loop is same with timeChunk and segment locks, their throughput should be similar.\r\n\r\n#### Add\r\n\r\n![add - varying # of segments](https://user-images.githubusercontent.com/2322288/61318564-37986280-a7ba-11e9-9f32-655580ab3245.png)\r\n![add - 160 segments](https://user-images.githubusercontent.com/2322288/61318568-38c98f80-a7ba-11e9-85bb-e4eac6b3082b.png)\r\n\r\nFor `add`, it's faster with segment lock because I think `TimelineEntry` should be created for each new segment of new major version. When using segment lock, new segment of new minor version can be added into the same `TimelineEntry`.\r\n\r\n#### Remove\r\n\r\n![remove - varying # of segments](https://user-images.githubusercontent.com/2322288/61318573-3a935300-a7ba-11e9-81ca-57b0968f13c0.png)\r\n![remove - 160 segments](https://user-images.githubusercontent.com/2322288/61318578-3bc48000-a7ba-11e9-907b-cd073af734da.png)\r\n\r\n`remove` is slower with segment lock. This is expected since whenever a visible segment is removed, `OvershadowableManager` needs to check all overshadowed atomicUpdateGroups to determine the visible one."}}
{"actor":{"display_login":"jihoonson"}}