OvershadowableManager.isComplete() looks buggy

[leventov]

The current implementation of OvershadowableManager.isComplete():
https://github.com/apache/incubator-druid/blob/a95e3d438e676310522a68c74a43d99f5f0492d5/core/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java#L891-L894

looks buggy because it might throw an exception when any value in visibleGroup is a map with more than one element. If there are never more than one element in this map, why this is a map, not just a single value?

It might be this this is not a bug because there are some invisible contract between callers to OvershadowableManager.isComplete() and OvershadowableManager which guarantee that when isComplete() is called all values in visibleGroup are maps of at most one value. But such a contract would need to be expressed explicitly in comments, at least. (Not to say that it is very fragile design and should preferably be avoided).

FYI @jihoonson

[jihoonson]

I understand this may be confusing. This is not a bug, but may be a missing comment on visibleGroups. The value type ofvisibleGroup is SingleEntryShort2ObjectSortedMap which can hold at most a single entry. This was to use the same interface with other groups such as standbyGroups and overshadowedGroups so that I can reuse the same help methods across all groups.

[leventov]

Thanks. I relabel this issue to Improvement to indicate that comments should be added. Also, it may be related to #8168.