kfaraz commented on code in PR #15952:
URL: https://github.com/apache/druid/pull/15952#discussion_r1508886811
##########
processing/src/main/java/org/apache/druid/timeline/partition/PartitionHolder.java:
##########
@@ -60,14 +67,24 @@ public PartitionHolder(List<PartitionChunk<T>>
initialChunks)
}
}
- protected PartitionHolder(OvershadowableManager<T> overshadowableManager)
+ protected PartitionHolder(OvershadowableManager<T> overshadowableManager,
short maxMinorVersion)
{
this.overshadowableManager = overshadowableManager;
+ this.maxMinorVersion = maxMinorVersion;
}
public boolean add(PartitionChunk<T> chunk)
{
- return overshadowableManager.addChunk(chunk);
+ boolean added = overshadowableManager.addChunk(chunk);
Review Comment:
Yes, we would have to maintain counts for each minor version.
So if we skip the impl on `remove`, if a minor version was ever added to the
timeline, we will continue to assume that that minor version is still present
for the purposes of computing maxMinorVersion. So we might sometimes enter into
the slow code flow even though it could have been avoided. I guess that's not
so bad.
For the timeline in the `DataSourcesSnapshot`, the impl in `remove` doesn't
matter (at least in the current implementation of the
`SqlSegmentsMetadataManager`) since objects are never removed, only added to
the timeline.
I guess we can keep the impl simple for now, as you suggest, but we should
atleast call it out in a comment.
Also, since the logic of the `maxMinorVersion` is very adhoc and confusing,
we should add comments as appropriate and keep the changes limited to the
`VersionedIntervalTimeline` class.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
