Improve fetch of pending segments from metadata store

[kfaraz]

The fetch of pending segments happens behind a lock and can cause others threads to remain stuck while deserializing the payload fetched from the metadata store.

The query to fetch the payload uses a <= and >= on the start and end intervals.

SELECT payload FROM pending_segments 
WHERE datasource = 'search-ds'
AND start <= 'search-end-time'
AND end >= 'search-start-time'

This might often end up fetching more segments than would actually have an overlap.
For example, when searching for the interval 10am to 11am, a segment with interval
9am to 10am would also satisfy this query.

start <= search-end AND end >= search-start
9 <= 11 AND 10 >= 10

But in reality, the interval 9am to 10am doesn't have any overlap with our search interval 10am to 11am.
This is because the end time of a segment interval is always exclusive.

Such results eventually get filtered out by the check on interval interval.overlaps(identifier.getInterval())
but only after the costly deserialization.

Changes:

• Improve select query to fetch only segments that can overlap.
  Use the same condition as present in Interval.overlaps

Notes:
This query (and also the original query) can give incorrect results in cases where a string comparison
of the two intervals is not feasible, typically if the search year and interval year are far apart.
See #11582 for a related fix.
A long-term fix for this could be to migrate these timestamps to use longs rather than varchars.

---

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[kfaraz]

Changed to emphasize that the interval used in the SegmentIdWithShardSpec is always the same as the one passed to the method createNewSegment.

[abhishekagarwal87]

LGTM. are there tests already for this method?

[rohangarg]

Regarding the suggested query improvement, I think BETWEEN is an inclusive operator on both sides so it might also bring the same results as the current query. Also, the performance in worst case might be slower.
Incase we want to eventually do a Interval#overlaps over the queried interval with the intervals in the pending segments table rows, we could follow the logic in that method and push it down in the metadata store query. The Interval#overlaps function is as follows :

public boolean overlaps(ReadableInterval interval) {
        long thisStart = getStartMillis();
        long thisEnd = getEndMillis();
        if (interval == null) {
            long now = DateTimeUtils.currentTimeMillis();
            return (thisStart < now && now < thisEnd);
        }  else {
            long otherStart = interval.getStartMillis();
            long otherEnd = interval.getEndMillis();
            return (thisStart < otherEnd && otherStart < thisEnd);
        }
}

[kfaraz]

Thanks for the suggestion, @rohangarg !
So, I guess we can just remove the equals from both the operators.

[kfaraz]

With the suggested change to the query, I don't think we even need the other changes. What do you think @abhishekagarwal87 , @AmatyaAvadhanula ?

[abhishekagarwal87]

I feel that there might be some nuances being missed here. The comparison in java code, that @rohangarg is referring to, is done on long millis representation of start and end. The start and end in the table are strings in yyyy-MM-ddTHH:mm:ss.SSSZZ. You should refer to https://github.com/apache/druid/pull/11582/files and try out some whacky dates in that PR. The string-based comparison doesn't work for all kinds of dates.

[kfaraz]

That makes sense, @abhishekagarwal87 . Looking at the other linked PR though, it is possible that even the current query would fail for dates with years that have more or less than 4 digits or even negative years (which would happen with ALL granularity). I will put all of this to the test and fix up the PR accordingly.

[rohangarg]

It seems like VARCHAR column based comparison for date-times in pendingSegments table could provide incorrect results in the current code as well. For correctness, the utilities introduced by #11582 could be used to fetch rows from pendingSegments table as well.
Although, I think for long run the schema of the tables could be augmented to have the date-time columns as TIMESTAMP or long to represent a UTC time instant (the augmentation could follow the approach implemented by @AmatyaAvadhanula in #12404)

[AmatyaAvadhanula]

Nit: Do start and end still need to be selected in the query?

[kfaraz]

Thanks for catching it, yeah, we can remove this now.

[gianm]

It looks like this PR has changed a bunch since the original description; it doesn't accurately describe the change anymore. @kfaraz could you please update it?

[gianm]

This looks like the only material change in the patch: what's the idea behind changing <= to < and what's the expected benefit?

[kfaraz]

We are trying to avoid selecting segments with intervals that only abut the search interval and don't have an actual overlap. Such intervals eventually get filtered out via the check on interval.overlaps(identifier.getInterval()) but only after the costly deserialization.

I have updated the description, which gives an example of such an interval.
This is still not a complete fix as the query can return incorrect results in cases where a string comparison of the two intervals is not feasible.

[gianm]

Thanks for clarifying.