Clarification regarding Appenderator's behavior of persist subset of segments

[pjain1]

AppenderatorImpl provides persist(Collection<SegmentIdentifier> identifiers, Committer committer) method which allows to persist only the segments passed in as input argument. So, if this method is called with subset of segments that AppenderatorImpl is handling then in that case the commitHydrants metadata persisted on disk will only have hydrants corresponding to this subset of segments. Is this expected behavior or the persist should try to merge previously committed hydrant information with the currently committing hydrants ?

To give some more context, when KafkaIndexTask allows incremental handoffs then it might call persist with subset of segments that are being published. There are two ways to handle this case -

1. Make sure the currently committed hydrants metadata gets merged with the previously committed metadata, also make sure that the caller commit metadata (next partitionOffsets information) is in line with what is being committed. This is what I have done for now, but it can get a little confusing to understand.
2. Keep the current behavior of persist in which it just overwrites previously committed metadata. To enable publishing subset of segments, add a new method pushWithoutPersist in Appenderator that pushes without persisting (currently persistAll is called implicitly), in this case make sure to call persistAll explicitly from the task before calling pushWithoutPersist so that it is guaranteed that the segments that needs to be published are all persisted.

Any opinions - @gianm @jihoonson

[jihoonson]

I think both approaches make sense. If you are going with the first approach, I think it's the better one. Also please check IndexTask. IndexTask is another task type which does incremental publishing. I think it should be fine with this change because it publishes all segments once some conditions meet, but maybe you need to fix or change something.

BTW, implicit persist() in push() looks weird to me as well because, as you said, it forces persisting segments whenever pushing them which is an inevitable side effect. I think simply push() should not call persist() implicitly, and callers should be responsible for deciding when segments are persisted.

[pjain1]

@jihoonson IndexTask does not restore on restart and I think all this commit metadata business gets used when Appenderator restores the state on restart. So, I guess IndexTask should be fine ?

[jihoonson]

You're correct. It should be fine.

[pjain1]

Fixed in #4815