SOLR-16412 : Race condition in SizeLimitedDistributedMap for cleanup #1032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
@noblepaul @chatman Can you have a review on this please? 😊 |
risdenk
reviewed
Oct 19, 2022
solr/core/src/test/org/apache/solr/cloud/TestSizeLimitedDistributedMap.java
Outdated
Show resolved
Hide resolved
Removed unused import
|
@risdenk would you mind to take a second look please? Many thanks! 🙇🏼 |
risdenk
approved these changes
Oct 24, 2022
| zookeeper.delete(dir + "/" + child, -1, true); | ||
| if (onOverflowObserver != null) | ||
| onOverflowObserver.onChildDelete(child.substring(PREFIX.length())); | ||
| } catch (KeeperException.NoNodeException e) { |
There was a problem hiding this comment.
nit: naming the exception ignore or ignored I think makes IDEs not care that the exception isn't used. It doesn't change the behavior at all just clear we meant to ignore the exception.
|
Can someone with write access please merge if it's all good? 😊 |
|
thanks @patsonluk |
noblepaul
pushed a commit
that referenced
this pull request
Oct 26, 2022
noblepaul
pushed a commit
that referenced
this pull request
Oct 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/SOLR-16412
Description
Details of the issue is described in the JIRA issue linked above.
Although we could enforce synchronization to prevent threads from purging the same set of child nodes, it might not be desirable to add extra blocking.
Instead, we should be more forgiving
SizeLimitedDistributedMap#shrinkIfNeededif any items/nodes in the map no longer exist.Solution
Catch
KeeperEx1ception.NoNodeExceptioninSizeLimitedDistributedMap#shrinkIfNeededif such node is already deleted, which is likely triggered by concurrentshrinkIfNeededcall.Tests
Added unit test case
TestSizeLimitedDistributedMap#testConcurrentCleanup, it's shown that without the current fix, the exception will be thrownChecklist
Please review the following and check all that apply:
mainbranch../gradlew check.