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

increase worker number to 4096 #260

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

haristku
Copy link
Contributor

@haristku haristku commented Oct 9, 2024

Description:

breaking down the previous PR to specific part

  • increase worker number to 4096

lang/en.json Outdated Show resolved Hide resolved
@AltamashShaikh
Copy link
Contributor

@haristku FYI, I tried fixing the failing tests but 1 test is failing randomly after this change, I would spend more time tomorrow to see if we can fix that test or not.

@AltamashShaikh
Copy link
Contributor

@haristku Its not allowing me to push, can you update plugins/QueuedTracking/tests/Integration/Queue/ManagerTest.php with this file ?

@haristku
Copy link
Contributor Author

@AltamashShaikh

@haristku Its not allowing me to push, can you update plugins/QueuedTracking/tests/Integration/Queue/ManagerTest.php with this file ?

updated... 👍

@snake14
Copy link
Contributor

snake14 commented Oct 13, 2024

@haristku It's looking good and it works as expected when I run it in my local environment. Can you please update these lines of SettingsTest to be the following?

        $this->expectExceptionMessage('The value should be at most 4096');

        $this->settings->numQueueWorkers->setValue('4097');

@snake14
Copy link
Contributor

snake14 commented Oct 14, 2024

@haristku It's looking good and it works as expected when I run it in my local environment. Can you please update these lines of SettingsTest to be the following?

        $this->expectExceptionMessage('The value should be at most 4096');

        $this->settings->numQueueWorkers->setValue('4097');

Never mind. Looks like Altamash got it 👍

Copy link
Contributor

@AltamashShaikh AltamashShaikh left a comment

Choose a reason for hiding this comment

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

Looks good to me, we can update the UI tests once the PR is merged

@AltamashShaikh
Copy link
Contributor

@snake14 Cam you check if we can merge this PR ?

@snake14
Copy link
Contributor

snake14 commented Oct 14, 2024

@snake14 Cam you check if we can merge this PR ?

@AltamashShaikh Yep. With all builds passing except UI, I think we're good to merge.

Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Changes look good, no issues found during functional testing, and all but UI build are passing 👍

@snake14 snake14 merged commit 39e49bd into matomo-org:5.x-dev Oct 14, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants