KAFKA-5872: Fix transient failure in SslSelectorTest.testMuteOnOOM #3836
Conversation
|
@radai-rosenblatt @ijuma Can you review, please? Thank you! |
| @@ -498,6 +498,7 @@ void pollSelectionKeys(Set<SelectionKey> selectionKeys, | |||
|
|
|||
| if (!outOfMemory && memoryPool.availableMemory() < lowMemThreshold) { | |||
| List<SelectionKey> temp = new ArrayList<>(selectionKeys); | |||
There was a problem hiding this comment.
Doesn't look like we need this temp variable. We can just assign it to inHandlingOrder directly, right? Also, using an ArrayList seems suboptimal given that we call remove on each element during iteration (each time shifting all the elements). ArrayDeque would work well if we didn't remove via the Iterator. Maybe we should change pollSelectionKeys to clear the collection at the end instead?
There was a problem hiding this comment.
I think the temp was possibly added since Collections.shuffle() needs a list. I have left that as-is. Since we only need to clear the selected key set, I have added clear to the end of poll() and got rid of Iterator.remove. It makes the code clearer as well.
| @@ -498,6 +498,7 @@ void pollSelectionKeys(Set<SelectionKey> selectionKeys, | |||
|
|
|||
| if (!outOfMemory && memoryPool.availableMemory() < lowMemThreshold) { | |||
| List<SelectionKey> temp = new ArrayList<>(selectionKeys); | |||
| selectionKeys.clear(); // needed for next `select` to count these keys | |||
There was a problem hiding this comment.
I think I'd mention in the comment that we normally remove from selectionKeys during iteration, but since we are making a copy here, we need to clear it manually.
There was a problem hiding this comment.
Removed this change.
|
@ijuma Thanks for the review, I have updated the code. |
| @@ -409,6 +409,9 @@ public void poll(long timeout) throws IOException { | |||
| // Add to completedReceives after closing expired connections to avoid removing | |||
| // channels with completed receives until all staged receives are completed. | |||
| addToCompletedReceives(); | |||
|
|
|||
| // Clear all selected keys so that they are included in the ready count for the next select | |||
| this.nioSelector.selectedKeys().clear(); | |||
There was a problem hiding this comment.
Wouldn't it be better to do this right after the last pollSelectionKeys? We can even just do readyKeys.clear() then.
There was a problem hiding this comment.
pollSelectionKeys is called multiple times with different key sets. Not all of them need clearing. We only really need to clear nioSelector.selectedKeys(). I thought it was clearer this way.
There was a problem hiding this comment.
Misread the comment earlier, updated.
rajinisivaram
force-pushed
the
KAFKA-5872-sslselectortest-failure
branch
from
e8faa81
to
a938fb7
Compare
|
@ijuma Thank you for the review, merging to trunk. |
|
LGTM, batch clearing is probably more efficient to boot |
No description provided.