time based checkpointing for Kafka Indexing Service

[pjain1]

Resolves #4815 (comment)

[jihoonson]

Would you add some tests?

[pjain1]

@jihoonson added

[dclim]

👍

[jihoonson]

Does this mean that time-based intermediate handoff is effectively disabled if it is null? What do you think about disabling it explicitly if it's null?

[jihoonson]

Also, please check the formatting.

[pjain1]

Fixed formatting and yes it means the intermediate handoff is effectively disabled. I didn't wanted to have another flag check so that's why just setting it to a long period. If you see any issues with this then I can change.

[jihoonson]

I think, from the user perspective, it looks not clear if they don't want to use this feature. If someone doesn't want to use it, then he/she might be confused what value should be set and should calculate how long period should be set to turn off this feature.
If we support turning it off explicitly in some ways, then it will be more clear for users. I think this is more valuable even though we need to add some more flag checks. Finally, I think it's enough if null intermediateHandoffPeriod means disabling time-based intermediate handoff. What do you think?

[pjain1]

How about just mentioning in the docs that null is default value and it means disabled. Internally, the null can be translated to a very long Period effectively disabling the intermediate handoff, does this sounds good ? Or should I introduce an explicit flag ?

[jihoonson]

I think it would be nice if what the doc says and how the code actually works matches. If you think it's better to keep the current implementation, the doc also should say that the default intermediateHandoffPeriod is Integer.MAX_VALUE days as it currently does.

I don't have a strong feeling about this.

[pjain1]

@jihoonson any more comments ?

[jihoonson]

Sorry for the late review. Looks good to me.