jihoonson commented on issue #7571: Optimize coordinator API to retrieve segments with overshadowed status URL: https://github.com/apache/incubator-druid/issues/7571#issuecomment-494510520 > Added to 0.15 milestone because If SegmentWithOvershadowedStatus will leak into Druid 0.15 API, it will be much harder to remove it from later This sounds like going back to feature-based release instead of time-based one. Per the [discussion](https://groups.google.com/forum/#!msg/druid-development/QPZUIzLtZ2I/hc3jkMATCgAJ;context-place=searchin/druid-development/roman%7Csort:date) you started, we are trying to freeze code every 3 months. After code freeze, we are backporting only regression bug fixes, security bug fixes, and doc changes if necessary. If you want to talk about a better release policy, please start a discussion on dev mailing list. Since this issue is not any of bug fix or doc change, I don't think this issue should be necessarily a blocker for 0.15.0 release. IMO, if it's not done yet before code freeze, it simply should wait for the next release. This is the least controversial way to choose features to add to each release I think. Especially for this issue, even the discussion on the proper solution is still ongoing. > Regarding DataSegment's immutability, it might need to go away anyway, as #6358 should be fixed (note @surekhasaharan: you can fix that problem, incl. across Coordinator -> Broker segment streaming, in the same PR). I agree the current way that DataSegment is updated and maintained is somewhat unintuitive. However, still I think it's not a good idea to make it immutable. Currently the variables can be updated in DataSegment are `loadSpec` and `size`, and the update happens only one time after the segment created by stream ingestion tasks is published. So, it's not the update of state, but update of metadata. `overshadowedStatus` is different. It represents a state of a dataSegment which can have different values depending on the context. This means, the `overshadowedStatus` must be computed correctly before it's used. If it's added to DataSegment as a mutable variable, I guess it would be very confusing unless there is a way to figure out under what context that state was computed. So, I'm also curious about the benefit we can when `DataSegment` becomes mutable. Is there anything else except memory savings? Finally, I don't think it's a good idea to recommend to fix several problems in a single PR. It makes the PR complicated which in turn making review harder. I would say, if the author wants to fix several problems in a single PR, I'm ok with that if the PR is not much complicated and large. But, I think simple PRs more likely get reviewed because they are easy to review.
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
