HDDS-15486 Remove excessive debug logging in BlockOutputStream and Bu…#10439
HDDS-15486 Remove excessive debug logging in BlockOutputStream and Bu…#10439yandrey321 wants to merge 2 commits into
Conversation
| releaser.start(); | ||
| allocator.join(); | ||
| assertEquals(toRelease, allocated.get()); | ||
| assertTrue(logCapturer.getOutput().contains("Allocation needs to wait the pool is at capacity")); |
There was a problem hiding this comment.
I think we are losing testing coverage if you remove this line.
We might be able to do the following above
assertTrue(pool.allocated.size() == pool.capacity)
allocated.set(pool.allocateBuffer(0));
However I am not completely sure.
To be safe, we can keep this type of verification. Other removals over LOG_DEBUG look ok.
There was a problem hiding this comment.
I'd avoid validation through log messages and prefer to do it via actual state check
There was a problem hiding this comment.
there is a check if pool is full at the beginning of the function:
assertFull(pool);
There was a problem hiding this comment.
Good point. Yes assertFull(pool) does state checks.
I am wondering if other people can also take a look at this before we merge. cc @adoroszlai.
This is just in case if there was hidden context for such log based testing that we do not know. And certainly if bugs exist in such code path (e.g. a while loop + wait in BufferPool), it is relatively hard to debug.
| // As the pool is full, allocation will need to wait until a buffer is released. | ||
| assertFull(pool); | ||
|
|
||
| assertEquals(buffers.size(), pool.getAllocatedBuffers().size()); |
There was a problem hiding this comment.
Looks like this addition verifies if there is any new allocation happened after a batch of buffer allocations through the pool.
Can we name this assertion asassertNoBuffersChangeInPool(buffersSize, pool)? This might have better readability.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@yandrey321 , Why removing the debug messages? They can be enabled/disabled by setting the log level of BlockOutputStream.
When there is a bugs, debug messages are very helpful.
3 participants
Remove excessive debug logging in BlockOutputStream and BufferPool
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15486
How was this patch tested?
Unit tests