From 6b9260f5d1ec57812a925d9ab90361a29cc275f2 Mon Sep 17 00:00:00 2001 From: Zhongjie Wu Date: Thu, 13 Sep 2012 11:03:42 -0700 Subject: [PATCH] Modified behavior of getall to comply javadoc when key does not exist in store --- .../action/PerformParallelGetAllRequests.java | 16 ++++---- .../action/PerformSerialGetAllRequests.java | 10 +++-- test/unit/voldemort/server/EndToEndTest.java | 4 +- .../voldemort/store/AbstractStoreTest.java | 16 +------- .../store/routed/GetallNodeReachTest.java | 37 ++++++------------- .../store/routed/RoutedStoreTest.java | 8 ++-- 6 files changed, 31 insertions(+), 60 deletions(-) diff --git a/src/java/voldemort/store/routed/action/PerformParallelGetAllRequests.java b/src/java/voldemort/store/routed/action/PerformParallelGetAllRequests.java index 357f30c97f..e44cfdc9d3 100644 --- a/src/java/voldemort/store/routed/action/PerformParallelGetAllRequests.java +++ b/src/java/voldemort/store/routed/action/PerformParallelGetAllRequests.java @@ -16,7 +16,6 @@ package voldemort.store.routed.action; -import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; @@ -138,19 +137,18 @@ public void requestComplete(Object result, long requestTime) { successCount.increment(); List> retrieved = values.get(key); - if(retrieved == null) { - retrieved = new ArrayList>(); - } /* * retrieved can be null if there are no values for the key * provided */ - List> existing = pipelineData.getResult().get(key); + if(retrieved != null) { + List> existing = pipelineData.getResult().get(key); - if(existing == null) - pipelineData.getResult().put(key, Lists.newArrayList(retrieved)); - else - existing.addAll(retrieved); + if(existing == null) + pipelineData.getResult().put(key, Lists.newArrayList(retrieved)); + else + existing.addAll(retrieved); + } HashSet zoneResponses = null; if(pipelineData.getKeyToZoneResponse().containsKey(key)) { diff --git a/src/java/voldemort/store/routed/action/PerformSerialGetAllRequests.java b/src/java/voldemort/store/routed/action/PerformSerialGetAllRequests.java index 0d8b551d4b..01c52d06d2 100644 --- a/src/java/voldemort/store/routed/action/PerformSerialGetAllRequests.java +++ b/src/java/voldemort/store/routed/action/PerformSerialGetAllRequests.java @@ -121,10 +121,12 @@ public void execute(Pipeline pipeline) { else values = store.get(key, transforms.get(key)); - if(result.get(key) == null) - result.put(key, Lists.newArrayList(values)); - else - result.get(key).addAll(values); + if(values.size() != 0) { + if(result.get(key) == null) + result.put(key, Lists.newArrayList(values)); + else + result.get(key).addAll(values); + } Map>> map = new HashMap>>(); map.put(key, values); diff --git a/test/unit/voldemort/server/EndToEndTest.java b/test/unit/voldemort/server/EndToEndTest.java index f8e9c798c8..7616e2ae61 100644 --- a/test/unit/voldemort/server/EndToEndTest.java +++ b/test/unit/voldemort/server/EndToEndTest.java @@ -1,8 +1,8 @@ package voldemort.server; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; import java.io.IOException; import java.util.ArrayList; @@ -120,7 +120,7 @@ public void testSanity() { assertEquals("getAll works as expected", "Moscow", capitals.get("Russia").getValue()); assertEquals("getAll works as expected", "Kiev", capitals.get("Ukraine").getValue()); - assertTrue("getAll works as expected", capitals.get("Japan") == null); + assertFalse("getAll works as expected", capitals.containsKey("Japan")); storeClient.delete("Ukraine"); assertNull("delete works as expected", storeClient.get("Ukraine")); diff --git a/test/unit/voldemort/store/AbstractStoreTest.java b/test/unit/voldemort/store/AbstractStoreTest.java index e2adf72f01..c157d1d1be 100644 --- a/test/unit/voldemort/store/AbstractStoreTest.java +++ b/test/unit/voldemort/store/AbstractStoreTest.java @@ -312,21 +312,7 @@ public void testGetAll() throws Exception { public void testGetAllWithAbsentKeys() throws Exception { Store store = getStore(); Map>> result = store.getAll(getKeys(3), null); - boolean resultZero = (result.size() == 0); - boolean resultEmpty = true; - if(!resultZero) { - if(result.get(result.keySet().toArray()[0]).size() != 0) { - resultEmpty = false; - } - if(result.get(result.keySet().toArray()[1]).size() != 0) { - resultEmpty = false; - } - if(result.get(result.keySet().toArray()[2]).size() != 0) { - resultEmpty = false; - } - } - assertTrue(resultZero || resultEmpty); - + assertEquals(0, result.size()); } @Test diff --git a/test/unit/voldemort/store/routed/GetallNodeReachTest.java b/test/unit/voldemort/store/routed/GetallNodeReachTest.java index 869c4a0ed1..39d28f43a0 100644 --- a/test/unit/voldemort/store/routed/GetallNodeReachTest.java +++ b/test/unit/voldemort/store/routed/GetallNodeReachTest.java @@ -2,7 +2,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static voldemort.VoldemortTestConstants.getEightNodeClusterWithZones; import static voldemort.VoldemortTestConstants.getFourNodeClusterWithZones; @@ -96,16 +95,15 @@ public void testGetallTouchOneZone() throws Exception { assertEquals(2, store.getAll(keys011, null) .get(TestUtils.toByteArray("k011_zone0_only")) .size()); - assertEquals(0, store.getAll(keys100, null) - .get(TestUtils.toByteArray("k100_zone1_only")) - .size()); + assertFalse(store.getAll(keys100, null) + .containsKey(TestUtils.toByteArray("k100_zone1_only"))); /* test multiple keys getall */ List keys = new ArrayList(); keys.add(TestUtils.toByteArray("k011_zone0_only")); keys.add(TestUtils.toByteArray("k100_zone1_only")); Map>> result = store.getAll(keys, null); assertEquals(2, result.get(TestUtils.toByteArray("k011_zone0_only")).size()); - assertEquals(0, result.get(TestUtils.toByteArray("k100_zone1_only")).size()); + assertFalse(result.containsKey(TestUtils.toByteArray("k100_zone1_only"))); } @Test @@ -161,13 +159,9 @@ public void testGetall_211() throws Exception { keys.add(TestUtils.toByteArray("k111")); Map>> result = store.getAll(keys, null); assertFalse(result.containsKey(TestUtils.toByteArray("not_included"))); - assertEquals(0, result.get(TestUtils.toByteArray("k000")).size()); - assertEquals(1, result.get(TestUtils.toByteArray("k001")).size()); - assertEquals(0, result.get(TestUtils.toByteArray("k010")).size()); + assertFalse(result.containsKey(TestUtils.toByteArray("k000"))); assertEquals(1, result.get(TestUtils.toByteArray("k011")).size()); - assertEquals(0, result.get(TestUtils.toByteArray("k100")).size()); - assertEquals(1, result.get(TestUtils.toByteArray("k101")).size()); - assertEquals(0, result.get(TestUtils.toByteArray("k110")).size()); + assertFalse(result.containsKey(TestUtils.toByteArray("k100"))); assertEquals(1, result.get(TestUtils.toByteArray("k111")).size()); } @@ -227,21 +221,12 @@ public void testGetall_211_zoneCountRead_1() throws Exception { Map>> result = store.getAll(keys, null); assertFalse(result.containsKey(TestUtils.toByteArray("not_included"))); /* client will first try all the nodes in local zone */ - assertEquals(0, result.get(TestUtils.toByteArray("k000")).size()); - assertEquals(1, result.get(TestUtils.toByteArray("k001")).size()); - // don't know which one of node 0 or 1 comes second on preferece list - // for key - // k*10 or k*01 - // therefore it can be 1 or 0 beside existence on node 2 - int size = -1; - size = result.get(TestUtils.toByteArray("k010")).size(); - assertTrue(size == 1 || size == 0); + assertFalse(result.containsKey(TestUtils.toByteArray("k000"))); + assertEquals(1, result.get(TestUtils.toByteArray("k011")).size()); + assertFalse(result.containsKey(TestUtils.toByteArray("not_included"))); + assertFalse(result.containsKey(TestUtils.toByteArray("k000"))); assertEquals(1, result.get(TestUtils.toByteArray("k011")).size()); assertEquals(1, result.get(TestUtils.toByteArray("k100")).size()); - size = result.get(TestUtils.toByteArray("k101")).size(); - assertTrue(size == 1 || size == 2); - size = result.get(TestUtils.toByteArray("k110")).size(); - assertTrue(size == 1 || size == 2); assertEquals(2, result.get(TestUtils.toByteArray("k111")).size()); } @@ -297,9 +282,9 @@ public void testGetall_322() throws Exception { keys.add(TestUtils.toByteArray("k1111_1111")); Map>> result = store.getAll(keys, null); assertFalse(result.containsKey(TestUtils.toByteArray("not_included"))); - assertEquals(0, result.get(TestUtils.toByteArray("k0000_0000")).size()); + assertFalse(result.containsKey(TestUtils.toByteArray("k0000_0000"))); assertEquals(2, result.get(TestUtils.toByteArray("k0000_1111")).size()); - assertEquals(0, result.get(TestUtils.toByteArray("k1111_0000")).size()); + assertFalse(result.containsKey(TestUtils.toByteArray("k1111_0000"))); assertEquals(2, result.get(TestUtils.toByteArray("k1111_1111")).size()); } } diff --git a/test/unit/voldemort/store/routed/RoutedStoreTest.java b/test/unit/voldemort/store/routed/RoutedStoreTest.java index 1b1ed1428e..975420379f 100644 --- a/test/unit/voldemort/store/routed/RoutedStoreTest.java +++ b/test/unit/voldemort/store/routed/RoutedStoreTest.java @@ -457,8 +457,8 @@ public void testZoneRouting() throws Exception { new ByteArray("test2".getBytes())); Map>> values = s1.getAll(keys, null); - List> results = values.get(new ByteArray("test".getBytes())); - assertEquals("\'test\' did not get deleted.", 0, results.size()); + assertFalse("'test' did not get deleted.", + values.containsKey(new ByteArray("test".getBytes()))); ByteUtils.compare(values.get(new ByteArray("test2".getBytes())).get(0).getValue(), new byte[] { 1 }); @@ -517,8 +517,8 @@ public void testZoneRouting() throws Exception { } values = s2.getAll(keys, null); - results = values.get(new ByteArray("test".getBytes())); - assertEquals("\'test\' did not get deleted.", 0, results.size()); + assertFalse("'test' did not get deleted.", + values.containsKey(new ByteArray("test".getBytes()))); ByteUtils.compare(values.get(new ByteArray("test2".getBytes())).get(0).getValue(), new byte[] { 1 });