[improve] [client] Prevent reserve memory with a negative memory size to avoid send task stuck

[poorbarcode]

Motivation

Context : The previous PR is #21790

Background of MemoryLimitController

• When there is not enough memory to use, the method MemoryLimitController.reserveMemory will stuck until some memory is released^[1].
• After calling MemoryLimitController.releaseMemory, it would wake up all stuck threads.

Issue

• the method tryReserveMemory, reserveMemory, forceReserveMemory will also release the memory if it accepted a negative value, but it will not wake up the stuck threads. This would cause the stuck threads to remain stuck forever.
• [fix][client] Reserve allocated buffer in BatchMessageContainer on client memory limitation. #17936 used this method incorrectly and caused a Producer stuck bug that fixed by [fix][client] Fix producer thread block forever on memory limit controller #21790.

Make sure that all use cases are correct

• reserveMemory
  • used by ProducerImpl.canEnqueueRequest, will not set a negative argument
• tryReserveMemory
  • used by ProducerImpl.canEnqueueRequest, will not set a negative argument
• forceReserveMemory
  • used by BatchMessageContainerImpl.updateAndReserveBatchAllocatedSize, will not set a negative argument
  • used by ConsumerBase.enqueueMessageAndCheckBatchReceive, will not set a negative argument

Just like @lhotari's suggestion, we should add a check to prevent new PRs incorrectly using tryReserveMemory, reserveMemory, forceReserveMemory

Footnotes

[1]:
https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MemoryLimitController.java#L88

if (!tryReserveMemory(size)) {
    while (!tryReserveMemory(size)) {
        condition.await();
    }
}

Modifications

Just like @lhotari's suggestion, add validation to prevent calling tryReserveMemory, reserveMemory, forceReserveMemory with a negative argument.

Documentation

• doc
• doc-required
• doc-not-needed
• doc-coplete

Matching PR in forked repository

PR in forked repository: x

[wenbingshen]

Thanks.

[lhotari]

@poorbarcode I think that it would make sense to guard the release memory methods with a similar check that ensures that the value is positive.

[poorbarcode]

@lhotari

I think that it would make sense to guard the release memory methods with a similar check that ensures that the value is positive.

Good suggestion. Added

[lhotari]

LGTM

[AnonHxy]

Nit： Zero is not positive but can pass this method. So change to reservedMemorySize <= 0 here or change the method name:)

[poorbarcode]

Good suggestion, skipped all logic if the argument is 0

[Technoboy-]

checkPositive

[poorbarcode]

Done

[Technoboy-]

And I think we can add checkPositive instead of ensureReservedMemoryIsPositive for the needed method. Error log seems redundant and makes this class ugly

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4bfc786) 73.07% compared to head (8544705) 73.54%.
Report is 18 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #21804      +/-   ##
============================================
+ Coverage     73.07%   73.54%   +0.47%     
+ Complexity    32573    32251     -322     
============================================
  Files          1897     1858      -39     
  Lines        140622   138040    -2582     
  Branches      15487    15116     -371     
============================================
- Hits         102754   101528    -1226     
+ Misses        29783    28645    -1138     
+ Partials       8085     7867     -218     

Flag	Coverage Δ
inttests	24.17% <47.36%> (-0.10%)	⬇️
systests	23.71% <63.15%> (-1.01%)	⬇️
unittests	72.84% <89.47%> (+0.51%)	⬆️

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

Files	Coverage Δ
...ache/pulsar/client/impl/MemoryLimitController.java	93.75% <89.47%> (-1.81%)	⬇️

... and 181 files with indirect coverage changes