Re: SegmentId PR

[Roman Leventov <le...@apache.org>]

It's now almost three months since this PR was opened. Could any committer
(non necessarily Gian) please provide a design review?

I want to remind that in PRs labelled "Design review", the second review is
not required to be full, a line-by-line textual review, it could literally
be just "design review". So in the context of the SegmentId PR, which Egor
has already reviewed in full, it should not be as time-consuming as
reviewing some other +3700/-3700 line PR. A big portion of that are
mechanical API changes across the project, in tests and benchmarks. As I
noted here:
https://github.com/apache/incubator-druid/pull/6370#issuecomment-423731847,
there are only 6 classes that are added or changed meaningfully.

On Mon, 12 Nov 2018 at 21:24, Roman Leventov <le...@apache.org> wrote:

> Because in a lot of places, SegmentId is deserialized from "segment id
> string", that doesn't have enough information to reconstruct the ShardSpec.
>
> On Mon, 12 Nov 2018 at 17:28, Gian Merlino <gi...@apache.org> wrote:
>
>> I could take a look after 0.13.0 is released. Right now things related to
>> that are the main things I am spending my Druid-related time on.
>>
>> I haven't read most of the diff yet, but I was wondering, is there a
>> reason
>> you make a new class instead of using SegmentIdentifier? They are slightly
>> different (one has a ShardSpec and one just has the partition num) but I
>> am
>> wondering if these need to be two different classes or not.
>>
>> On Mon, Nov 12, 2018 at 4:51 AM Roman Leventov <le...@apache.org>
>> wrote:
>>
>> > Could somebody please provide design review of "Introduce SegmentId
>> class"
>> > PR : https://github.com/apache/incubator-druid/pull/6370? This is an
>> > important improvement, and many other improvements and bugs fixes are
>> > blocked on it. Despite "Development Blocker" tag (that was thought to
>> give
>> > PRs a priority), nobody reviewed this PR for almost two months, except
>> Egor
>> > with whom we work for the same company.
>> >
>>
>