gianm commented on issue #7571: Optimize coordinator API to retrieve segments with overshadowed status URL: https://github.com/apache/incubator-druid/issues/7571#issuecomment-496450211 @leventov, I think I understand what you are suggesting: that BrokerServerView and MetadataSegmentView might both create their own DataSegment objects (well, really, they'd create a subclass: SegmentWithOvershadowedStatus) and, for such objects where (a) `isOvershadowed=false` and (b) they were first created by BrokerServerView, the objects will be shared because MetadataSegmentView will check with BrokerServerView to see if it has any reusable objects. (Or, perhaps, [b] might not be a requirement, if the checking is bidirectional: BrokerServerView could also check with MetadataSegmentView.) It sounds fine, but I think keeping SegmentWithOvershadowedStatus as a wrapper object makes more sense. There's some additional pointer overhead, but it should be low. And there are benefits: - The wrapper object approach allows DataSegment objects to _always_ be shared (not just when `overshadowed=false`). - The wrapper object approach avoids the "weird at first glance but actually fine" characteristic in BrokerServerView or the need to protect against misuse. - Senses of equality (or deep equality, as the case may be) can be murky when inheritance is involved. Is a SegmentWithOvershadowedStatus equal to a DataSegment if every field is equal other than `overshadowed`? Which behavior is best probably depends on where you are in the code. Composition through wrappers makes it very clear how the comparison works. I think these benefits outweigh the memory savings of removing one layer of wrapping in MetadataSegmentView. What do you all think? --------------- Side notes: By the way, I do think the idea to switch from interners to probing-based reuse is good and we should do that (although it doesn't need to be a 0.15.0 blocker). And on the topic of https://github.com/apache/incubator-druid/issues/6358#issuecomment-489102183, it sounds like a good idea to me (DataSegment's `equals` behavior is a perennial source of confusion; it generally makes more sense to use SegmentId) but it should be doable separately, and also doesn't need to be a 0.15.0 blocker. Maybe some piece of it might be implemented as part of these changes (the deeplyEquals / allDataEquals could be useful) but the structural inspection stuff is better suited for another day.
---------------------------------------------------------------- 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]
