gianm commented on issue #7571: Optimize coordinator API to retrieve segments with overshadowed status URL: https://github.com/apache/incubator-druid/issues/7571#issuecomment-495800919 > Immutable SegmentWithOvershadowedStatus which extends DataSegment (note: they also have different JSON representations). I guess this means we need to get rid of DataSegmentInterner (DataSegment objects cannot be interned effectively if they are being subclassed and have `overshadowed` fields that need to be changed). So we would want to add some other way of sharing objects between the broker's timeline (ServerSelector) and the metadata segment cache (MetadataSegmentView). We could perhaps have those two things store SegmentId objects rather than DataSegment objects, and have them use a shared registry object that stores the actual DataSegments. However, there are some wrinkles here: 1. The 'shared registry object' would probably be some kind of wrapper around `Map<SegmentId, SegmentWithOvershadowedStatus>`. But when ServerSelector needs to register a DataSegment, it _doesn't have_ a valid overshadowed status. (It's just coming from a segment announcement, not a timeline, and 'overshadowed status' only makes sense in the context of a timeline.) 2. There are not one, but _two_ timelines: the timeline of published segments, and the timeline of available segments. They don't always match up 100%. So in the single SegmentWithOvershadowedStatus object, which timeline is being referred to? One way to answer these questions is: 1. The 'shared registry' should wrap a `Map<SegmentId, DataSegment>`, where the DataSegments may be SegmentWithOvershadowedStatus or they may be the base class DataSegment. If they are the base class, it means we lack information about overshadowed status (maybe, so far, we've only seen it in a segment announcement). If they are the class SegmentWithOvershadowedStatus, it means we do have that information. We'll need to differentiate with `instanceof` checks. 2. It should refer to the timeline of published segments. (That was the original motivation for introducing all this stuff in the first place.) What do you all think - @leventov, @surekhasaharan, @jihoonson, @jon-wei? By the way, the JSON representations would just be different in that SegmentWithOvershadowedStatus has some extra fields, right? > Although immutable, both are decidedly containers, not "data classes". They both prohibit equals() and hashCode() as described here (https://github.com/apache/incubator-druid/issues/6358#issuecomment-489102183). What do you mean by 'prohibit' @leventov? I followed the link, and I don't see a suggestion about prohibiting equals and hashCode. (Just about prohibiting using DataSegments as keys in a map, which seems reasonable, given that _usually_ people probably meant to use SegmentId.)
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
