Make DynamicFilter future resilient to cancel

[raunaqmorarka]

Cancelling CompletableFuture returned by DynamicFilter#isBlocked
should not affect dynamic filter collection. If cancellation of
a future from a consumer of DynamicFilter is allowed to propgate
into DynamicFilterService or LocalDynamicFiltersCollector,
then it will prevent the completion of collection of that
dynamic filter and it's usage by other consumers.

[findepi]

Can you please add rationale to the commit message?

Also, would it be possible to cover the rogue effects of cancellation with an integration test too?

[sopel39]

lgtm % comments

[raunaqmorarka]

Can you please add rationale to the commit message?

Also, would it be possible to cover the rogue effects of cancellation with an integration test too?

@findepi I've updated the commit message with a more detailed description.
If TestDynamicFilterService#testDynamicFilterCancellation is run without the change to wrap futures in unmodifiableFuture (and some asserts around future state are ignored), then we can see that dynamic filter is marked complete even though the collection is not complete. We also get Same future set twice error when setting lazyDynamicFilters in addDynamicFilters in the storeSummary step after cancellation of future.

[findepi]

then we can see that dynamic filter is marked complete even though the collection is not complete.

should we cancel the collection (and release associated resources) when connector cancels the DF future?

[sopel39]

should we cancel the collection (and release associated resources) when connector cancels the DF future?

Same future might be used by multiple connectors. It would require us tracking that all DF consumers cancelled that future. We then could potentially forget that particular DF. However, we fetch DFs from workers regardless. I think such mechanism would incur extra complexity which might not be needed at this point.

[findepi]

Same future might be used by multiple connectors.

Like in

- Join
  - Union All
     - connector 1
     - connector 2

?

[sopel39]

Like in

More like:

- Join
  - Join
    - TS
    - TS
  - TS

but union works too

[sopel39]

@findepi if you don't have any more comments, I will merge it

[sopel39]

merged, thanks!