fix(polling/state): prevent duplicates in repeatable requests list

[romansmirnov]

Description

Related issues

relates #8310
closes #8390

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• New content is added to the release announcement

[romansmirnov]

I know, I know... using HashSet without implementing hashCode() and equals() in LongPollingActivateJobsRequest 😄 But none of the existing given properties (each on its own and all together) helps to identify a request clearly. For example, a replicated worker shares the same configuration which would result in multiple separate requests with the same request property values. So that, the requests could not be distinguished anymore as they would be considered as equal.

Possible Solutions:

1. Continue using HashSet but introduce a unique identifier for each incoming request per job type. The ID could be used to implement equals() and hashCode().
2. Keep the type LinkedList but check if the request is already contained by the list

activeRequests.forEach((r) -> {
  if (!activeRequestsToBeRepeated.contains(r)) {
    activeRequestsToBeRepeated.add(r);
  }
});

3. Accept the duplicates as they all will be removed when the request times out (but the request might be executed multiple times unnecessarily)
4. ...

[romansmirnov]

Dear Reviewer,

Please consider the following:

• Introduce a requestId property in LongPollingActivateJobsRequest which allows to clearly identify a request. Additionally, that allows to implement #hashCode() and #equals() accordingly. Otherwise, the existing properties do not allow to differentiate them properly.
• The requestId is not persisted, meaning, after a Gateway restart the requestId starts again with 1.

Also, the PR might be in conflict with the PR #8389 when merging it.

[romansmirnov]

Dear Reviewer, just FYI: I consciously decided to use the CountDownLatch to ensure that all requests are submitted to the actor initially. As an alternative, I considered using the ControlledActorScheduler but the test was too flaky with the controlled scheduler. With the latch, the test is reliable.

[lenaschoenburg]

❌ I think introducing a request id is not necessary because we can just rely on identity to check for duplicates in activeRequestsToBeRepeated. We create new instances of LongPollingActivateJobsRequest only once.

That means activeRequestsToBeRepeated could be:

private final Set<LongPollingActivateJobsRequest> activeRequestsToBeRepeated =
      Collections.newSetFromMap(new IdentityHashMap<>());

Unless we want to use the request id for more than just checking duplicates I think we should not introduce it here and instead rely on object identity.

WDYT?

[npepinpe]

Regarding identity, what you say sounds reasonable - as long as we only create requests once and each instance is meant to be unique, it should be OK to simply use the default equals. I can't really speak on his intentions and reasoning, but I can imagine two reasons why Roman would've implemented a custom one however (which would require an ID as otherwise there's no way to guarantee we can distinguish requests):

1. What if that constraint changes? It's not a specific constraint of the class that it must be instantiated only once per request, so it's not too crazy to think someone might not respect it.
2. What if later on, someone thinks: "hey, what if we aggregated similar requests that share the same properties/subset to reduce load on the broker? we could implement a custom equals/hashCode and put that in a set so they're automatically deduplicated per job type". This may break the current expectations of uniqueness, and maybe it wouldn't be immediately obvious what happened, or easy to diagnose.

That's just OTOH though - my proposal would be to accept with the ID, and open a follow up to discuss it with Roman when he's back - there's no rush here, and removing the ID or taking a different approach in the long run is easy to do. Let me know what you think 👍

[lenaschoenburg]

I rebased to resolve conflicts with the other gateway/long-polling related PRs.

bors r+

[ghost]

Build succeeded:

• continuous-integration/jenkins/branch

[github-actions]

Successfully created backport PR #8453 for stable/1.1.

[github-actions]

Successfully created backport PR #8454 for stable/1.2.

[github-actions]

Backport failed for release-1.3.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-1.3.0
git worktree add -d .worktree/backport-8392-to-release-1.3.0 origin/release-1.3.0
cd .worktree/backport-8392-to-release-1.3.0
git checkout -b backport-8392-to-release-1.3.0
ancref=$(git merge-base 2c754a7d19d50455a1e790260477d03a2e760137 460fc6fe5ecf871aa100bcfa10d9f8bd8ec460c9)
git cherry-pick -x $ancref..460fc6fe5ecf871aa100bcfa10d9f8bd8ec460c9