{"action":"created","issue":{"url":"https://api.github.com/repos/apache/incubator-druid/issues/6370","repository_url":"https://api.github.com/repos/apache/incubator-druid","labels_url":"https://api.github.com/repos/apache/incubator-druid/issues/6370/labels{/name}","comments_url":"https://api.github.com/repos/apache/incubator-druid/issues/6370/comments","events_url":"https://api.github.com/repos/apache/incubator-druid/issues/6370/events","html_url":"https://github.com/apache/incubator-druid/pull/6370","id":362842043,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE3NDQ4Njk3","number":6370,"title":"Introduce SegmentId class","user":{"login":"leventov","id":609240,"node_id":"MDQ6VXNlcjYwOTI0MA==","avatar_url":"https://avatars0.githubusercontent.com/u/609240?v=4","gravatar_id":"","url":"https://api.github.com/users/leventov","html_url":"https://github.com/leventov","followers_url":"https://api.github.com/users/leventov/followers","following_url":"https://api.github.com/users/leventov/following{/other_user}","gists_url":"https://api.github.com/users/leventov/gists{/gist_id}","starred_url":"https://api.github.com/users/leventov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leventov/subscriptions","organizations_url":"https://api.github.com/users/leventov/orgs","repos_url":"https://api.github.com/users/leventov/repos","events_url":"https://api.github.com/users/leventov/events{/privacy}","received_events_url":"https://api.github.com/users/leventov/received_events","type":"User","site_admin":false},"labels":[{"id":592291319,"node_id":"MDU6TGFiZWw1OTIyOTEzMTk=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Design%20Review","name":"Design Review","color":"f9d0c4","default":false},{"id":664694515,"node_id":"MDU6TGFiZWw2NjQ2OTQ1MTU=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Development%20Blocker","name":"Development Blocker","color":"b60205","default":false},{"id":321808195,"node_id":"MDU6TGFiZWwzMjE4MDgxOTU=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Improvement","name":"Improvement","color":"0052cc","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":120288359,"node_id":"MDU6TGFiZWwxMjAyODgzNTk=","url":"https://api.github.com/repos/apache/incubator-druid/labels/Performance","name":"Performance","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-09-22T09:36:52Z","updated_at":"2018-09-30T13:01:01Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/apache/incubator-druid/pulls/6370","html_url":"https://github.com/apache/incubator-druid/pull/6370","diff_url":"https://github.com/apache/incubator-druid/pull/6370.diff","patch_url":"https://github.com/apache/incubator-druid/pull/6370.patch"},"body":"This PR introduces the `SegmentId` class for operating with segment ids in the codebase instead of Strings.\r\n\r\nThree reasons:\r\n - Memory efficiency: `SegmentId`'s footprint is about 50 bytes smaller than a String, assuming non-compressed Oops, if I calculated correctly (should be ~25 bytes smaller since Java 9's \"compact strings\"). The difference should be bigger with compressed Oops.\r\n - Code readability and safety: it's more readable and safer to operate with `Map<SegmentId, Something>` than with `Map<String, Something>`\r\n - `SegmentId` enabled to get rid of the `segments` map in `DruidServer` and `ImmutableDruidServer`, for additional memory savings.\r\n\r\nAlso:\r\n - Replaced `ConcurrentSkipListMap` with `ConcurrentHashMap` in `DruidDataSource`, because it doesn't really need to be sorted.\r\n - Improved concurrency of `DruidServer`.\r\n - Improved concurrency of `SQLMetadataSegmentManager`.\r\n\r\nCaveats of `SegmentId` introduction:\r\n - HTTP endpoints that return a lot of segment ids as parts of some JSON answers generate more garbage, because they convert `SegmentId` objects into Strings. Previously, those Strings were already stored in the heap.\r\n - HTTP endpoints which accept segment id need to do more work, because they need to iterate all possible parsings of a segment id string into `SegmentId`, that is ambiguous. But this should be minor, compared to the cost of an HTTP call itself. The exception is `MetadataResource.getDatabaseSegmentDataSourceSegment()`: to preserve it's former behaviour precisely, it would require to generate quite a lot of garbage `String` objects on each endpoint call, so the logic of this endpoint was changed (see below).\r\n\r\nChanged HTTP endpoint behaviour:\r\n - `MetadataResource.getDatabaseSegmentDataSourceSegment()`, `/datasources/{dataSourceName}/segments/{segmentId}`: it used to search for segment id match ignoring the case of the string (for reference, it dates back to this commit: https://github.com/apache/incubator-druid/commit/3cba0c8ff2ab89e20e0707cd4f585e0aab2e0e90#diff-1fad8d6f3e6e3a17b71cd63bd21353e9R153), that's not in line with all other segmentId-accepting endpoints, and to me it doesn't look correct, because `\"ver\"` and `\"VER\"` are two different versions according to the rest of the logic. In the PR, the endpoint is changed to look for case-sensitive segmentId match.\r\n - `DatasourcesResource.getSegmentDataSourceSpecificInterval()`, `/{dataSourceName}/intervals/{interval}`, `simple` option: it creates a `HashMap` of intervals, unlike same method, `full` option, and both `simple` and `full` options of `getSegmentDataSourceIntervals()`, which all create `TreeMap`s of intervals. To me it looked like a slip, so I changed this endpoint to create a `TreeMap` as well (i. e. return interval JSON maps in different order the endpoint does currently).\r\n\r\nThis PR is labelled `Incompatible` because it changed multiple Public APIs (to return `SegmentId` instead of `String` from some getters)."},"comment":{"url":"https://api.github.com/repos/apache/incubator-druid/issues/comments/425719493","html_url":"https://github.com/apache/incubator-druid/pull/6370#issuecomment-425719493","issue_url":"https://api.github.com/repos/apache/incubator-druid/issues/6370","id":425719493,"node_id":"MDEyOklzc3VlQ29tbWVudDQyNTcxOTQ5Mw==","user":{"login":"leventov","id":609240,"node_id":"MDQ6VXNlcjYwOTI0MA==","avatar_url":"https://avatars0.githubusercontent.com/u/609240?v=4","gravatar_id":"","url":"https://api.github.com/users/leventov","html_url":"https://github.com/leventov","followers_url":"https://api.github.com/users/leventov/followers","following_url":"https://api.github.com/users/leventov/following{/other_user}","gists_url":"https://api.github.com/users/leventov/gists{/gist_id}","starred_url":"https://api.github.com/users/leventov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leventov/subscriptions","organizations_url":"https://api.github.com/users/leventov/orgs","repos_url":"https://api.github.com/users/leventov/repos","events_url":"https://api.github.com/users/leventov/events{/privacy}","received_events_url":"https://api.github.com/users/leventov/received_events","type":"User","site_admin":false},"created_at":"2018-09-30T13:01:01Z","updated_at":"2018-09-30T13:01:01Z","author_association":"MEMBER","body":"@egor-ryashin to get the picture of functional changes it should be enough to review closely the classes mentioned here: https://github.com/apache/incubator-druid/pull/6370#issuecomment-423731847. Changes in other places are more or less mechanical."}}
{"actor":{"display_login":"leventov"}}