Load rules should honor partial overlap #5595
Conversation
| } | ||
|
|
||
| public static boolean eligibleForLoad(Period period, Interval interval, DateTime referenceTimestamp) | ||
| { | ||
| final Interval currInterval = new Interval(period, referenceTimestamp); | ||
| return currInterval.overlaps(interval) && interval.getStartMillis() >= currInterval.getStartMillis(); | ||
| return currInterval.overlaps(interval); |
There was a problem hiding this comment.
nit: better to call eligibleForLoad(currInterval, interval) for easier code maintenance.
|
@palanieppan-m thanks for your contribution! Would you fill CLA (http://druid.io/community/cla.html)? |
| } | ||
|
|
||
| public static boolean eligibleForLoad(Period period, Interval interval, DateTime referenceTimestamp) | ||
| { | ||
| final Interval currInterval = new Interval(period, referenceTimestamp); | ||
| return currInterval.overlaps(interval) && interval.getStartMillis() >= currInterval.getStartMillis(); |
There was a problem hiding this comment.
Does anyone remember or know why && interval.getStartMillis() >= currInterval.getStartMillis() was here in the first place? I can't think of a good reason for it - probably you'd want a load rule to apply if it overlaps a segment all, even if the segment doesn't start within the load rule interval.
Load rules should load segments that partially overlap with rule window, instead of loading only segments that fully overlap.
palanieppan-m
force-pushed
the
pala_fix_bg_segment_merge_orig
branch
from
48c5562
to
b4fb3d0
Compare
|
@jihoonson good suggestion, changed accordingly. Also filled in CLA. Thanks |
|
@gianm @palanieppan-m I believe we've had this issue raised before and it was rejected at the time, see #1362 |
|
Thanks @xvrl. My feeling is that the justification given in #1362 for the current logic is not worth the unintuitive behavior in the case where a segment starts before the load interval but also extends into it. The intuitive thing would be to have the load rule apply if there is any overlap. I agree with the comment @cheddar made at the time,
My feeling is we should go ahead with this PR as-is and then also raise an issue to look into the router's behavior and potentially rethink it (or add a comment in TieredBrokerHostSelector). cc/ @fjy @cheddar @nishantmonu51 @anubhgup from #1362 |
|
can this be added to the 0.12.1 milestone? |
|
@pdeva added. |
|
@gianm I agree the current behavior is unintuitive and we should make the change. However, it still is a backwards incompatible change at the router level, so we should deal with it appropriately and note it in the release notes. |
|
IMO, it would be acceptable to merge this as-is and just mention in the release notes that router behavior in this case may change as a result. It sounds like I am in agreement with @xvrl on this. However I think on balance the behavior changes means that it makes sense to put this in 0.13.0 not 0.12.1. Has anyone else got an opinion on this? |
|
0.13.0 sounds like the reasonable choice here unless someone wants to do additional work to preserve the existing router behavior. |
|
@xvrl - I agree. I'm milestoning this to 0.13.0 and will merge it soon if there are no further comments. |
|
thanks folks. I will go ahead and port this into our internal branch for use now. |
Load rules should load segments that partially overlap with rule window, instead of loading only segments that fully overlap.
Load rules should load segments that partially overlap with rule window, instead of loading only segments that fully overlap.
Load rules should load segments that partially overlap with rule window,
instead of loading only segments that fully overlap. Otherwise, valid data is
skipped and result produced is incorrect.