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(gtw/jobs): ignore notifications if already scheduled #8317

Merged
1 commit merged into from
Dec 9, 2021

Conversation

romansmirnov
Copy link
Member

@romansmirnov romansmirnov commented Dec 3, 2021

Description

  • Ignore incoming notifications if a notification is already scheduled/running for the given job type
  • Changes the type from HashMap to ConcurrentHashMap of field LongPollingActivateJobsHandler#jobTypeState because it is accessed by different threads
    • Actor Thread who does reads and updates to the map
    • "Netty Threads" who notifies the gateway when newly jobs are available

Related issues

closes #8267

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 romansmirnov force-pushed the 8267-gtw-notification branch 4 times, most recently from 8face80 to 5de9109 Compare December 9, 2021 10:48
@romansmirnov romansmirnov marked this pull request as ready for review December 9, 2021 12:26
@romansmirnov
Copy link
Member Author

@deepthidevaki, can you please review this PR? In the case, you can't make it this week anymore, then please let me know.

Regarding a unit test to cover that scenario, I tried different approaches to write a working unit test but did not succeed. If you see any option, how to write a test case, therefore, then please let me know. I am happy to receive your ideas.

Copy link
Contributor

@deepthidevaki deepthidevaki left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
I cannot think of a way to test it other than what we already discussed. Since the existing Long polling tests works, we can assume that it did not break the existing behavior. That would be enough.

Before merging this PR:

  1. Could you update the commit message to include a small test describing the reasoning behind these changes. Basically what you have added in the PR description.
  2. Have you already run a benchmark to ensure that this fixed the issue?

@romansmirnov
Copy link
Member Author

@deepthidevaki,

Thanks for your review.

Could you update the commit message to include a small test describing the reasoning behind these changes. Basically what you have added in the PR description.

Will do!

Have you already run a benchmark to ensure that this fixed the issue?

Yes, I run the benchmark today and last week, it's documented in the issue itself: #8267 (comment)

Copy link
Contributor

@deepthidevaki deepthidevaki left a comment

Choose a reason for hiding this comment

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

👍

* Ignore incoming notifications if a notification is already scheduled/running
  for the given job type
* Changes the type from `HashMap` to `ConcurrentHashMap` of field
  `LongPollingActivateJobsHandler#jobTypeState` because it is accessed by different threads:
  * Actor Thread who does reads and updates to the map
  * "Netty Threads" who notifies the gateway when newly jobs are available
@romansmirnov
Copy link
Member Author

bors r+

@ghost
Copy link

ghost commented Dec 9, 2021

Build succeeded:

@ghost ghost merged commit 8431348 into develop Dec 9, 2021
@ghost ghost deleted the 8267-gtw-notification branch December 9, 2021 16:15
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

Successfully created backport PR #8345 for stable/1.1.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

Successfully created backport PR #8346 for stable/1.2.

ghost pushed a commit that referenced this pull request Dec 16, 2021
8345: [Backport stable/1.1] fix(gtw/jobs): ignore notifications if already scheduled r=romansmirnov a=github-actions[bot]

# Description
Backport of #8317 to `stable/1.1`.

relates to #8267

Co-authored-by: Roman <roman.smirnov@camunda.com>
ghost pushed a commit that referenced this pull request Dec 16, 2021
8346: [Backport stable/1.2] fix(gtw/jobs): ignore notifications if already scheduled r=romansmirnov a=github-actions[bot]

# Description
Backport of #8317 to `stable/1.2`.

relates to #8267

8387: [Backport stable/1.2] fix(journal): always release acquired read lock r=romansmirnov a=github-actions[bot]

# Description
Backport of #8372 to `stable/1.2`.

relates to #8369

Co-authored-by: Roman <roman.smirnov@camunda.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too many workers can break cluster performance
3 participants