[CHANGED] PurgeDeletes() will now keep markers that are less than 30min old

[kozlovic]

There is a breaking change where PurgeDeletes() accepts now a list
of PurgeOpt, not WatchOpt.

We needed from WatchOpt only the context, and as it standed, it was
bad since user could have passed the IncludeHistory() option to
PurgeDeletes(), which would likely "break" the functionality.

Also, when invoking PurgeDeletes(), the delete markers than are older than
a default of 30 minutes will be deleted, however more recent ones will
be kept. The data is always removed, even if a marker is not.

The user can change the 30 minutes threshold using a new purge
option called DeleteMarkersOlderThan(duration). If set to -1,
it restores the old behavior of deleting delete/purge markers,
regardless of their age.

Signed-off-by: Ivan Kozlovic ivan@synadia.com

[derekcollison]

I like it, we should place under normal PurgeDeletes though IMO.

We can change over the WatchOpts possibly since I think we only use them for context here.

[coveralls]

Coverage increased (+0.04%) to 85.218% when pulling 4afdb30 on kv_purge_with_time into 3ead809 on main.

[derekcollison]

LGTM!

[kozlovic]

@derekcollison Yes, but we have a problem. This would work (ability to set the value) only in case where user calls CreateKeyValue(). For KeyValue() (lookup and bind), that won't work. So either we make it a kvs setter, or we pull Wally's PR (remove WatchOpt and add PurgeOpt) but then add the new logic with Keep:1.

[derekcollison]

Good point. I would say for now remove option to set and we can see how things play out if folks really want to change it. Those tombstones are small so as long as we remove the history itself and keep them should be fine with default of 30mins to start.

[kozlovic]

Ok, sounds good. I will keep it internally so that I can override for tests, but otherwise will not expose it.

Argh... tests are in test/ package, so I can't. Will have to move the test into nats package..

[kozlovic]

@derekcollison Ok, I have updated the PR by removing the option and move a test to nats package so I can override the 30min for the test. I also changed the PR's title to indicate the change in behavior (especially that it is not configurable atm).

[derekcollison]

LGTM

[kozlovic]

@derekcollison @wallyqs I think it would make sense to bring Wallys changes (from the gist) so we can have "purge options" to replace "WatchOpt" because otherwise users could really break the PurgeDeletes() call, say by passing IncludeHistory, etc.. Also, it would be nice to be able to set the option of this threshold.
So if you don't mind, I will take Wally's changes and update this PR.

[wallyqs]

LGTM!

[kozlovic]

@derekcollison @wallyqs Ok, last iteration I promise :-) I have taken Wally's changes (although I had to remove type purgeOptFn and its configurePurge receiver since they were not used - staticcheck would fail).
So this is still keeping the markers that are less than 30 minutes old by default, but can be changed through option. Also, removes use of WatchOpt for PurgeDeletes() which could have been mis-used.