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 isRamAlmostOverloaded function in Consumer #435

Merged
merged 1 commit into from
Jun 21, 2017

Conversation

agentsib
Copy link
Contributor

No description provided.

@agentsib
Copy link
Contributor Author

Hello, maybe need change something?

@stloyd stloyd added the Bug fix label May 29, 2017
@@ -175,6 +175,6 @@ protected function isRamAlmostOverloaded()
{
$memoryManager = new MemoryConsumptionChecker(new NativeMemoryUsageProvider());

return $memoryManager->isRamAlmostOverloaded($this->getMemoryLimit(), '5M');
return $memoryManager->isRamAlmostOverloaded($this->getMemoryLimit().'M', '5M');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to check & adjust if already set memory limit is not in such format.

Choose a reason for hiding this comment

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

This check is already done by convertHumanUnitToNumerical() method.

Just a suggestion:
Alter/better documentation in the ReadMe
Its unclear the memory-limit / -l option is not in MB's by default.

The example -l 256 is 256 bytes and not 256 mega bytes.
so for reference -l=256M will be the correct notation or -l=268435456

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adm-bome Ok, I'll do it as soon as I have time. Thanks/

@stloyd stloyd merged commit c14de14 into php-amqplib:master Jun 21, 2017
@stloyd
Copy link
Collaborator

stloyd commented Jun 21, 2017

Thanks @agentsib !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants