From 3c1d4b4a4cf35aec31c03e3c0ddd8af348fc6c38 Mon Sep 17 00:00:00 2001 From: Sashidhar Thallam Date: Sat, 5 Oct 2019 13:45:13 +0530 Subject: [PATCH 1/2] Fix for 8614: iterators returned by strategy don't share iteration state. --- ...dRobinStorageLocationSelectorStrategy.java | 7 ++- .../StorageLocationSelectorStrategyTest.java | 59 ++++++++++++++++--- 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java b/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java index 362fb8c0eca8..c757b87acaaa 100644 --- a/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java +++ b/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java @@ -48,6 +48,7 @@ public Iterator getLocations() private final int numStorageLocations = storageLocations.size(); private int remainingIterations = numStorageLocations; + private int i = startIndex.getAndUpdate(n -> (n + 1) % numStorageLocations); @Override public boolean hasNext() @@ -62,8 +63,10 @@ public StorageLocation next() throw new NoSuchElementException(); } remainingIterations--; - final StorageLocation nextLocation = - storageLocations.get(startIndex.getAndUpdate(n -> (n + 1) % numStorageLocations)); + final StorageLocation nextLocation = storageLocations.get(i++); + if (i == numStorageLocations) { + i = 0; + } return nextLocation; } }; diff --git a/server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java b/server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java index bfe79cafb7a0..2795083f29b4 100644 --- a/server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java +++ b/server/src/test/java/org/apache/druid/segment/loading/StorageLocationSelectorStrategyTest.java @@ -110,29 +110,74 @@ public void testRoundRobinLocationSelectorStrategy() throws Exception StorageLocationSelectorStrategy roundRobinStrategy = new RoundRobinStorageLocationSelectorStrategy(storageLocations); - iterateLocs(localStorageFolder1, localStorageFolder2, localStorageFolder3, roundRobinStrategy); - iterateLocs(localStorageFolder1, localStorageFolder2, localStorageFolder3, roundRobinStrategy); - iterateLocs(localStorageFolder1, localStorageFolder2, localStorageFolder3, roundRobinStrategy); - iterateLocs(localStorageFolder1, localStorageFolder2, localStorageFolder3, roundRobinStrategy); - iterateLocs(localStorageFolder1, localStorageFolder2, localStorageFolder3, roundRobinStrategy); + // First call to getLocations() + Iterator locations = roundRobinStrategy.getLocations(); + + StorageLocation loc1 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1", + localStorageFolder1, loc1.getPath()); + + StorageLocation loc2 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_2", + localStorageFolder2, loc2.getPath()); + + StorageLocation loc3 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_3", + localStorageFolder3, loc3.getPath()); + + + // Second call to getLocations() + locations = roundRobinStrategy.getLocations(); + + loc2 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_2", + localStorageFolder2, loc2.getPath()); + + loc3 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_3", + localStorageFolder3, loc3.getPath()); + + loc1 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1", + localStorageFolder1, loc1.getPath()); } - private void iterateLocs(File localStorageFolder1, File localStorageFolder2, File localStorageFolder3, - StorageLocationSelectorStrategy roundRobinStrategy) + @Test + public void testRoundRobinLocationSelectorStrategyMultipleCallsToGetLocations() throws Exception { + List storageLocations = new ArrayList<>(); + + final File localStorageFolder1 = tmpFolder.newFolder("local_storage_folder_1"); + final File localStorageFolder2 = tmpFolder.newFolder("local_storage_folder_2"); + final File localStorageFolder3 = tmpFolder.newFolder("local_storage_folder_3"); + + storageLocations.add(new StorageLocation(localStorageFolder1, 10000000000L, null)); + storageLocations.add(new StorageLocation(localStorageFolder2, 10000000000L, null)); + storageLocations.add(new StorageLocation(localStorageFolder3, 10000000000L, null)); + + StorageLocationSelectorStrategy roundRobinStrategy = new RoundRobinStorageLocationSelectorStrategy(storageLocations); + Iterator locations = roundRobinStrategy.getLocations(); StorageLocation loc1 = locations.next(); Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1", localStorageFolder1, loc1.getPath()); + locations = roundRobinStrategy.getLocations(); + StorageLocation loc2 = locations.next(); Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_2", localStorageFolder2, loc2.getPath()); + locations = roundRobinStrategy.getLocations(); + StorageLocation loc3 = locations.next(); Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_3", localStorageFolder3, loc3.getPath()); + + loc1 = locations.next(); + Assert.assertEquals("The next element of the iterator should point to path local_storage_folder_1", + localStorageFolder1, loc1.getPath()); } } From f7b06bc65aaf87dfe989e21f888def0c362fe53a Mon Sep 17 00:00:00 2001 From: Sashidhar Thallam Date: Sat, 5 Oct 2019 13:53:58 +0530 Subject: [PATCH 2/2] Adding comments --- .../loading/RoundRobinStorageLocationSelectorStrategy.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java b/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java index c757b87acaaa..1dd666005c2d 100644 --- a/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java +++ b/server/src/main/java/org/apache/druid/segment/loading/RoundRobinStorageLocationSelectorStrategy.java @@ -48,6 +48,8 @@ public Iterator getLocations() private final int numStorageLocations = storageLocations.size(); private int remainingIterations = numStorageLocations; + // Each call to this methods starts with a different startIndex to avoid the same location being picked up over + // again. See https://github.com/apache/incubator-druid/issues/8614. private int i = startIndex.getAndUpdate(n -> (n + 1) % numStorageLocations); @Override