Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not retry create instance on closed connection #18264

Merged

Conversation

megglos
Copy link
Contributor

@megglos megglos commented May 6, 2024

Description

This replaces ConnectionException throws in cases where a connection was actually established previously with a more appropriate MessagingException.ConnectionClosed, as ConnectionException is only supposed to be used in cases where a connection couldn't get established at all, see https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/net/ConnectException.html.

Related issues

closes #17333

@github-actions github-actions bot added the component/zeebe Related to the Zeebe component/team label May 6, 2024
@megglos megglos force-pushed the meg-17333-do-not-retry-createInstance-on-closed-connection branch 2 times, most recently from 21e5a61 to 4764db9 Compare May 6, 2024 14:56
@megglos megglos force-pushed the meg-17333-do-not-retry-createInstance-on-closed-connection branch from 4764db9 to 9b68e09 Compare May 6, 2024 15:13
@megglos megglos marked this pull request as ready for review May 6, 2024 15:13
@megglos megglos added backport stable/8.2 backport stable/8.3 Backport a pull request to 8.3.x backport stable/8.4 Backport a pull request to 8.4.x backport stable/8.5 Backport a pull request to stable/8.5 benchmark and removed benchmark labels May 6, 2024
@megglos
Copy link
Contributor Author

megglos commented May 6, 2024

failed state is just due to a failed benchmark setup caused by the excessive branch name 🙃

@megglos
Copy link
Contributor Author

megglos commented May 6, 2024

I created a generation Zeebe PR 18264 based on this on integration and ran the https://github.com/pierre-yves-monnet/c8-highcreation test project against a cluster running it, it created 10k instances without a duplicate detected.

@megglos megglos requested a review from npepinpe May 6, 2024 15:49
Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no test for the NettyMessagingService change, but I can see that's difficult to test in a controlled way 🤔 Let's go with this 👌

Nice catch!

@megglos megglos added this pull request to the merge queue May 6, 2024
Merged via the queue into main with commit 00a85e0 May 6, 2024
63 of 66 checks passed
@megglos megglos deleted the meg-17333-do-not-retry-createInstance-on-closed-connection branch May 6, 2024 16:47
@backport-action
Copy link
Collaborator

Backport failed for stable/8.2, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin stable/8.2
git worktree add -d .worktree/backport-18264-to-stable/8.2 origin/stable/8.2
cd .worktree/backport-18264-to-stable/8.2
git switch --create backport-18264-to-stable/8.2
git cherry-pick -x 9b68e0937b3c2942cdda9372469e5facb1808158

@backport-action
Copy link
Collaborator

Successfully created backport PR for stable/8.3:

@backport-action
Copy link
Collaborator

Successfully created backport PR for stable/8.4:

@backport-action
Copy link
Collaborator

Successfully created backport PR for stable/8.5:

github-merge-queue bot pushed a commit that referenced this pull request May 6, 2024
…nection (#18284)

# Description
Backport of #18264 to `stable/8.5`.

relates to #17333
original author: @megglos
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2024
…nection (#18282)

# Description
Backport of #18264 to `stable/8.3`.

relates to #17333
original author: @megglos
github-merge-queue bot pushed a commit that referenced this pull request May 6, 2024
…nection (#18286)

# Description
Manual backport of #18264 to `stable/8.2`.

There were conflicts in
[MessagingException.java](https://github.com/camunda/zeebe/pull/18286/files#diff-cfd7f2bc0198b1461470a353efa3fbebce2cec3c7ca71c96513eefc316b61245)
which contains one subtype less than on all newer branches.

relates to #17333
original author: @megglos
github-merge-queue bot pushed a commit that referenced this pull request May 7, 2024
…nection (#18283)

# Description
Backport of #18264 to `stable/8.4`.

relates to #17333
original author: @megglos
@Zelldon Zelldon added version:8.4.7 Marks an issue as being completely or in parts released in 8.4.7 version:8.5.1 Marks an issue as being completely or in parts released in 8.5.1 version:8.3.11 Marks an issue as being completely or in parts released in 8.3.11 labels May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable/8.3 Backport a pull request to 8.3.x backport stable/8.4 Backport a pull request to 8.4.x backport stable/8.5 Backport a pull request to stable/8.5 component/zeebe Related to the Zeebe component/team version:8.3.11 Marks an issue as being completely or in parts released in 8.3.11 version:8.4.7 Marks an issue as being completely or in parts released in 8.4.7 version:8.5.1 Marks an issue as being completely or in parts released in 8.5.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential duplicate PI creation across partitions in case of request timeouts
4 participants