jiajunwang commented on issue #363: Fix the race condition while Helix refresh cluster status cache. URL: https://github.com/apache/helix/pull/363#issuecomment-516194738 > I think a unit test for verifying the race condition of propertyChangedMap is still needed. To manipulate the situation in an integration test would be much harder. My general suggestion is still the same, something like > > ``` > // create a resource data provider > class CustomHashMap { > Lock lock; > > // manually create race conditions in the implementation > } > > Map<> propertyChangeMap = new CustomHashMap(); > Lock lock = propertyChangeMap.getLock() > ResourceDataProvider provider = new ResourceDataProvider(propertyChangeMap) > ``` If we are going to add this test, it will be a very white box test. Basically, what we can test is how many times the map.get() is called in one typical cache refresh. So easier way is to count the map.get() calls. That actually serves the same target. With this change, as long as the caller is using getAndSet, we will be good. To summary, two reasons that I think the test is not necessary. 1. even you create the custom HashMap, the implementation can still get that value and call the get() multiple times, and the map won't know. 2. the cache code will be refactored soon, so these logics will be gone in most cases.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
