Repository navigation
Fix is_realtime column behavior in sys.segments table - #8154
Conversation
| final DruidServerMetadata realtimeServer = servers.stream() | ||
| .filter(metadata -> metadata.getType().equals(ServerType.REALTIME)) | ||
| .findAny() | ||
| .orElse(null); |
There was a problem hiding this comment.
Since findAny() returns an Optional, you could use Optional.isPresent() below instead of using orElse() to convert to null. Another option is to use anyMatch() instead of findAny().
There was a problem hiding this comment.
ah, sure, i used Optional.isPresent()
| .map(AvailableSegmentMetadata::getSegment) | ||
| .collect(Collectors.toList()); | ||
| Assert.assertEquals(segments.size(), 3); | ||
| Assert.assertEquals(segments.size(), 4); |
There was a problem hiding this comment.
If 4 is the expected value, it should be the first argument. (Similar comment for other occurrences below).
There was a problem hiding this comment.
fixed here and other places.
| .filter(segment -> segment.getDataSource().equals("foo3")) | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
There was a problem hiding this comment.
Do you want to assert existingSegment is not null? (similar comment for other occurrences below)
| .orElse(null); | ||
|
|
||
| final AvailableSegmentMetadata metadata = segmentsMetadata.get(existingSegment.getId()); | ||
| Assert.assertEquals(metadata.isRealtime(), 1L); |
There was a problem hiding this comment.
Make 1L the first argument as it's the expected value.
| .findAny(); | ||
|
|
||
| // if there is no realtime server in the replicas, isRealtime flag should be unset | ||
| long isRealtime = realtimeServer.isPresent() ? 1 : 0; |
There was a problem hiding this comment.
Apologies for missing this original discussion, but is this the most useful thing for the is_realtime flag to mean, where it reports true if any of the servers serving a segment is a realtime task?
It might be more useful for is_realtime to report true if it is only available on realtime tasks, and false if any server serving the segment is a historical, since that is how the broker will behave at query time (#4764). I think this would probably involve doing this as part of addSegment instead of removeSegment.
There was a problem hiding this comment.
You have a point, i think i sort of agree, it'll make is_realtime more intuitive for users. The docs for is_realtime here say this flag is 1 if this segment is being served on any type of realtime tasks, that'll need to change. Yeah, with the changed semantics of is_realtime, the flag will be unset when addSegment is called a second time via a historical. May be it should be updated in removeSegment as well, but the check will be if there is any historical server in the replicas, unset this flag.
There was a problem hiding this comment.
This change will make it incompatible with previous behavior, which I hope is okay, unless someone is relying on that behavior of is_realtime.
There was a problem hiding this comment.
It might be more useful for is_realtime to report true if it is only available on realtime tasks, and false if any server serving the segment is a historical
That sounds good to me
There was a problem hiding this comment.
I think the idea I had in the previous conversation was motivated by is_realtime being false meaning that the segment has completely flushed out of the realtime system. I thought it'd be useful for finding segments where handoff is lingering for some reason.
If we change it to what you are proposing, @clintropolis, then it'd be somewhat harder to find segments where handoff is lingering (but still possible by joining segments and servers). But it'd be easier to find segments where handoff has progressed far enough that a historical has loaded the segment.
Six of one, half dozen of the other.
I am okay with either approach, so long as it's clearly documented what the behavior should be. I don't think the fact that the change would be theoretically incompatible is a big deal, since the column is buggy enough that it doesn't work anyway right now.
|
👍 |
* Fix is_realtime flag * make variable final * minor changes * Modify is_realtime behavior based on review comment * Fix UT
Fixes #8142
Description
Update the
isRealtimeflag when a segment is dropped by a realtime task.In
DruidSchemaadded a check inremoveServerSegment()when none of the replicas is a realtime server type, markisRealtime=0.Added unit test.
This is marked
incompatiblebecause the semantics of theis_realtimeflag for a segment is changed, earlier a segment would be marked realtime, if any realtime task was serving the segment. It's now changed to that a segment would be marked realtime, if only realtime task is serving the segment and if a historical starts serving the segment, it's realtime flag is 0 or false.This PR has: