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: Omnichannel Jobs running every 5 seconds #32186

Merged
merged 9 commits into from
Apr 17, 2024

Conversation

KevLehman
Copy link
Contributor

@KevLehman KevLehman commented Apr 11, 2024

https://rocketchat.atlassian.net/browse/CORE-259

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

This is to reduce the frequency of these actions. These 3 schedulers were left behind when we moved away from synced-cron to our own cron package. Other task will be created to move these schedulers to use it in the future.

Copy link
Contributor

dionisio-bot bot commented Apr 11, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented Apr 11, 2024

🦋 Changeset detected

Latest commit: 29dac58

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.61%. Comparing base (6e0b80d) to head (29dac58).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32186      +/-   ##
===========================================
+ Coverage    48.45%   54.61%   +6.16%     
===========================================
  Files         1980     2289     +309     
  Lines        45294    50662    +5368     
  Branches      9153    10376    +1223     
===========================================
+ Hits         21948    27670    +5722     
+ Misses       21347    20512     -835     
- Partials      1999     2480     +481     
Flag Coverage Δ
e2e 53.34% <ø> (+13.03%) ⬆️
unit 75.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@KevLehman KevLehman marked this pull request as ready for review April 12, 2024 14:06
@KevLehman KevLehman requested review from a team as code owners April 12, 2024 14:06
@MarcosSpessatto
Copy link
Member

Since we are skipping the test covering this, we will create unit tests to cover the whole class in a subsequent PR.

Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

There is no way this could be a patch release, There is no problem being solved here, at most we are masking something worse. I don't believe we should release this as a fix.

Furthermore, the title points out to something different.

Skipping a test is not ideal, you could make this modifiable via an envvar

@KevLehman
Copy link
Contributor Author

Problem being solved is that the default timing for OM jobs is 5 secs, while the default timer for any other Agenda job (via @rocket.chat/cron) is 1 minute. These items were left when refactoring from old jobs to /cron package.

While I'd agree there's no "problem" being solved, the sole fact these jobs are going to the db every 5s (which also caused problems in the past when we first migrated to /cron) is problematic by itself.

What title would you suggest? Title says we're increasing agenda time, maybe that part is not clear but what we're actually doing is increasing the agenda processing time.

About the test, yeah, skipping is not ideal, but since this is targetted for an older version, we decided to not to add more stuff in here and just skip for now. Idea is to make these jobs configurable in the future, just not now.

wdyt?

@ggazzo
Copy link
Member

ggazzo commented Apr 15, 2024

I didn't understand the part about targeting an old version, as it is pointing to develop

about the title, chore, generally does not indicate fix, but some internal modification. Doesn't sound like a real fix

maybe something like fix: Omnichannel Jobs running every 5 seconds

@KevLehman KevLehman changed the title chore: Increase agenda time in Omnichannel EE jobs to 1 minute fix: Omnichannel Jobs running every 5 seconds Apr 15, 2024
@KevLehman
Copy link
Contributor Author

All right, updated PR title. I actually didn't notice I put this one as a chore. Fixed now.

The part about and old version is from a customer that's getting bombed by this. When we first moved to cron package, we had the same issue of DB getting bombed by agenda and so the fix was to update it to run every min. These 3 jobs were left behind on that fix (probably since the fix was spot on Open and we don't have these running there) and so the same issue persisted for these, and now a customer is having problems because of it (not all the problems are tied to these, but fixing them would alliviate a bit)

@scuciatto scuciatto added the stat: QA assured Means it has been tested and approved by a company insider label Apr 16, 2024
@KevLehman KevLehman added the stat: ready to merge PR tested and approved waiting for merge label Apr 16, 2024
@kodiakhq kodiakhq bot merged commit 1734bd0 into develop Apr 17, 2024
43 checks passed
@kodiakhq kodiakhq bot deleted the chore/increase-agenda-time-om-jobs branch April 17, 2024 18:05
@ggazzo
Copy link
Member

ggazzo commented Apr 18, 2024

/patch

Copy link
Contributor

dionisio-bot bot commented Apr 18, 2024

Pull request #32253 added to Project: "Patch 6.7.1"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants