Optimize coordinator API to retrieve segments with overshadowed status

[surekhasaharan]

Coordinator API /druid/coordinator/v1/metadata/segments?includeOvershadowedStatus builds VersionedIntervalTimeline which can be memory intensive and expensive if this API is called multiple times but multiple brokers in a cluster.

The comment here suggests :

• isOvershadowed becomes a non-final field of DataSegment object itself, not participating in equals() and hashCode().
• Add interface SegmentsAccess { ImmutableDruidDataSource prepare(String dataSource); Iterable<DataSegment> iterateAll(); } (strawman naming)
• Add DataSourceAccess computeOvershadowed() method to SQLSegmentMetadataManager, which performs this computation for every snapshot of SQLSegmentMetadataManager.dataSources (which is updated in poll()) at most once, lazily.
• Both endpoints in MetadataResource and Coordination balancing logic (which currently computes isOvershadowed status on its own, too) use this API.
• On the side of MetadataSegmentView, maintain something like a Map<DataSegment, DataSegment> and update overshadowed status like map.get(segmentFromCoordinator).setOvershadowed(segmentFromCoordinator.isOvershadowed())

[surekhasaharan]

@leventov I have some follow-up question on the suggested design.

isOvershadowed becomes a non-final field of DataSegment

So are we saying make DataSegment mutable and add a setter for overshadowed field ?

Add interface SegmentsAccess { ImmutableDruidDataSource prepare(String dataSource); Iterable iterateAll(); } (strawman naming)

What is the purpose of this interface, who would implement this ? Are you thinking SQLMetadataSegmentManager implements this ?

Add DataSourceAccess computeOvershadowed() method to SQLSegmentMetadataManager,

What is DataSourceAccess here ? Did you mean SQLMetadataSegmentManager ?

Both endpoints in MetadataResource and Coordination balancing logic (which currently computes isOvershadowed status on its own, too) use this API.

which API ? MetadataSegmentManager already has Iterable<DataSegment> iterateAllSegments();, what if this returns DataSegment with isOvershadowed field set, which can be set in SQLMetadataSegmentManager poll method.

On the side of MetadataSegmentView, maintain something like a Map<DataSegment, DataSegment> and update overshadowed status like map.get(segmentFromCoordinator).setOvershadowed(segmentFromCoordinator.isOvershadowed())

I think this will not be required, if coordinator API already returns DataSegment which has overshadowed status.

[gianm]

The general idea of computing overshadowedness as few times as possible sounds good to me, and in particular computing it on poll sounds good. I think we might as well do it eagerly, though, since it's always going to be needed (DruidCoordinatorRuleRunner, for example, always computes the list of overshadowed segments so it can avoid loading them on historicals).

I am skeptical of making DataSegment mutable, though. The alternative is a wrapper object: something like DataSegmentWithTimelineContext or DataSegmentWithOvershadowedState. The main motivation I could see for adding fields to DataSegment, vs. doing a wrapper object, is memory savings. (Is there some other reason I am missing?)

It is nice, I think, that DataSegment objects are self-contained -- they don't depend on other context. 'isOvershadowed' by its nature depends on other context (namely, what other DataSegments exist). It won't be possible to set the field correctly unless all DataSegment objects for a particular datasource are considered. This won't be possible in all cases, which will make it unclear when the field is valid and when it's not, opening up new possible bugs. A wrapper encodes into the type system whether or not the contextual overshadowedness information has been computed or not.

Furthermore, I am not convinced this tactic will be effective at minimizing memory use. As an example, the proposed Map<DataSegment, DataSegment> in MetadataSegmentView would need to keep two copies of each DataSegment, since the key and value are different objects. A wrapper would be cheaper, since the DataSegment object contained by different wrappers could be shared.

[leventov]

@surekhasaharan

So are we saying make DataSegment mutable and add a setter for overshadowed field ?

Yes

What is the purpose of this interface, who would implement this ? Are you thinking SQLMetadataSegmentManager implements this ?

The purpose is type-level and concurrency safety, however, since I agree with @gianm that it's easier to just compute overshadowed status eagerly on each poll(), this part of the suggestion is irrelevant. SQLMetadataSegmentManager's interface will be intact.

I think this will not be required, if coordinator API already returns DataSegment which has overshadowed status.

The idea is to keep using a segment object which resides the Broker's memory for longer, the same idea as here: https://github.com/apache/incubator-druid/blob/15d19f3059a990f022b1cb98bdd4e6a82cdf9a06/server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentManager.java#L750-L756

@gianm

I think we might as well do it eagerly

Yes. I now also think that instead of rebuilding VersionedIntervalTimeline every time from scratch, one can be kept in memory of SQLMetadataSegmentManager. (It would also allow to skip computations if no segments were added or removed in a particular data source during some poll(), compared to the previous.) The second part is optional.

I am skeptical of making DataSegment mutable, though. The alternative is a wrapper object: something like DataSegmentWithTimelineContext or DataSegmentWithOvershadowedState. The main motivation I could see for adding fields to DataSegment, vs. doing a wrapper object, is memory savings. (Is there some other reason I am missing?)

It is nice, I think, that DataSegment objects are self-contained -- they don't depend on other context. 'isOvershadowed' by its nature depends on other context (namely, what other DataSegments exist). It won't be possible to set the field correctly unless all DataSegment objects for a particular datasource are considered. This won't be possible in all cases, which will make it unclear when the field is valid and when it's not, opening up new possible bugs. A wrapper encodes into the type system whether or not the contextual overshadowedness information has been computed or not.

Looking at the existing code at #7425, I think wrapper approach would be worse.

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). DataSegment is immutable and used in HashSets and HashMaps as a key, while it's identity is actually based only on segmentId, is not right. We should probably move to Map<SegmentId, DataSegment> across the board and make DataSegment a mutable object without equals() and hashCode() implementations to avoid defects like #6358 consistently.

As an example, the proposed Map<DataSegment, DataSegment> in MetadataSegmentView would need to keep two copies of each DataSegment, since the key and value are different objects.

The idea was that key and value is the same object in that case, but anyway, it should rather be Map<SegmentId, DataSegment>, see above.

[leventov]

Probably all fields of DataSegment (except overshadowed) should remain final to avoid problematic concurrency updates when actually discovering an instance of #6358 problem (which should be very rare), but the general idea is the same: we should "attach less" to DataSegment's immutability, therefore, I don't see that having a single mutable field in it would be a problem.

[surekhasaharan]

I feel the mutability of DataSegment is still being debated, and the major concern is recomputation of overshadowed status and building VersionedIntervalTimeline on each coordinator API call. So I am addressing the latter with #7595

[leventov]

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 (it will require transition PRs through several consecutive Druid versions, and temporary glue code). So I would really like to see it go away before Druid 0.15 if other people agree that needs to be done. Please provide arguments if you disagree.

[jihoonson]

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 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.

[jihoonson]

For the metadata of segment, perhaps it's better to have different data structures for segments which are actively generated by stream ingestion tasks and others. For example, the former might not have loadSpec and size fields to reduce confusion. For interning properly, we might need two maps (segmentId -> segment metadata) for each data structure which their keys are never overlapped.

[gianm]

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 (it will require transition PRs through several consecutive Druid versions, and temporary glue code). So I would really like to see it go away before Druid 0.15 if other people agree that needs to be done. Please provide arguments if you disagree.

@leventov - which API leak are you concerned about? From context I'm guessing it's the HTTP API. I suggest we address that by not documenting it and treating it as an internal API. The info is still exposed through system tables (the original motivation for creating this HTTP API), and that won't change even if we end up wanting to alter the underlying API.

By the way, I'm still not really sure that a mutable DataSegment is the way to go. It just feels wrong. It's a class that is meant to represent the 'payload' in the druid_segments table and the announcement in ZK of an available segment. In my experience these sorts of widely used modeling classes work best when they are immutable. Also, if we added mutable fields like 'isOvershadowed', it would often be invalid (for example: in code that is reading or writing an individual segment announcement in ZK, where the overshadowedness concept does not have meaning).

Besides, I don't think that having mutable DataSegments would really do too much to reduce churn. DataSegments aren't updated very often. Most are either never updated, or are just updated once (during realtime-to-historical handoff). If someone is doing a deep storage migration they might update all their segments, but this would be quite infrequent. If we use wrappers in situations that want to track extra state, like overshadowedness, then those wrappers could be mutable to avoid churn in the wrappers. (Meaning: we could make SegmentWithOvershadowedStatus mutable.)

[leventov]

@jihoonson

This sounds like going back to feature-based release instead of time-based one. Per the discussion you started, we are trying to freeze code every 3 months.

Note that I unblocked #7425 only on the condition that people agree with the proposed approach and it's being implemented immediately: #7425 (comment). If I knew that new round of debate will start, I wouldn't unblock that PR and insisted on completely resolving that question before merging.

@gianm

From context I'm guessing it's the HTTP API. I suggest we address that by not documenting it and treating it as an internal API.

Internal API cannot be broken either to ensure there is a safe rolling upgrade/downgrade plan. So version 0.16 would need to add alternative implementation while fully supporting the old SegmentWithOvershadowedStatus, and only 0.17 can remove SegmentWithOvershadowedStatus.

---

I think the main reason why I proposed mutable isOvershadowed field was that I initially proposed to compute it lazily, on demand in SQLMetadataSegmentManager. However, later I agreed (in this thread, actually) that it can also be eager. So I think immutable would work fine.

I'm also curious about the benefit we can when DataSegment becomes mutable. Is there anything else except memory savings?

Memory and concurrency (see #7595).

So what about the following:

• Immutable, but extensible DataSegment.
• Immutable SegmentWithOvershadowedStatus which extends DataSegment (note: they also have different JSON representations).
• Although immutable, both are decidedly containers, not "data classes". They both prohibit equals() and hashCode() as described here.

[jihoonson]

Note that I unblocked #7425 only on the condition that people agree with the proposed approach and it's being implemented immediately: #7425 (comment). If I knew that new round of debate will start, I wouldn't unblock that PR and insisted on completely resolving that question before merging.

@leventov thank you for additional context. I didn't notice this before. This sounds like Both #7425 and #7595 are supposed to be done in a single PR, but they are separate for the sake of convenience of the author and reviewers. And I like to avoid two potential problems in #7425, i.e., slow API performance and internal API mismatch between versions. So, I think it makes sense to add this to 0.15.0 as long as #7595 won't take long time.

So what about the following:

• Immutable, but extensible DataSegment.
• Immutable SegmentWithOvershadowedStatus which extends DataSegment (note: they also have different JSON representations).
• Although immutable, both are decidedly containers, not "data classes". They both prohibit equals() and hashCode() as described here.

Your suggestion sounds good to me.

Memory and concurrency (see #7595).

#7595 looks about eager computation of overshadowed segments rather than mutable segments. Would you tell me more how the mutable segment helps concurrency?