abhishekrb19 commented on code in PR #17212:
URL: https://github.com/apache/druid/pull/17212#discussion_r1801260554
##########
extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java:
##########
@@ -73,14 +76,43 @@ public void testUnapplyNull()
}
@Test
- public void testApply() throws ExecutionException
+ public void testApply()
{
- EasyMock.expect(lookupCache.get(EasyMock.eq("key"),
EasyMock.anyObject(Callable.class))).andReturn("value").once();
+
EasyMock.expect(lookupCache.getIfPresent(EasyMock.eq("key"))).andReturn("value").once();
EasyMock.replay(lookupCache);
Assert.assertEquals(ImmutableMap.of("key", "value"),
loadingLookup.applyAll(ImmutableSet.of("key")));
EasyMock.verify(lookupCache);
}
+ @Test
+ public void testApplyWithNullValue()
+ {
+
EasyMock.expect(lookupCache.getIfPresent(EasyMock.eq("key"))).andReturn(null).once();
+ EasyMock.expect(dataFetcher.fetch("key")).andReturn(null).once();
+ EasyMock.replay(lookupCache, dataFetcher);
+ Assert.assertNull(loadingLookup.apply("key"));
+ EasyMock.verify(lookupCache, dataFetcher);
+ }
+
+ @Test
+ public void testApplyWithCacheCheck()
Review Comment:
```suggestion
public void testApplyTriggersCacheMissAndSubsequentCacheHit()
```
nit: you can drop the comments since the test name is descriptive enough.
##########
extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java:
##########
@@ -73,15 +75,22 @@ public String apply(@Nullable final String key)
return null;
}
- final String presentVal;
- try {
- presentVal = loadingCache.get(keyEquivalent, new
ApplyCallable(keyEquivalent));
+ final String presentVal = this.loadingCache.getIfPresent(keyEquivalent);
+ if (presentVal != null) {
return NullHandling.emptyToNullIfNeeded(presentVal);
}
- catch (ExecutionException e) {
- LOGGER.debug("value not found for key [%s]", key);
+
+ String val = this.dataFetcher.fetch(keyEquivalent);
+ if (val == null) {
return null;
}
+
+ Map<String, String> map = new HashMap<>();
+ map.put(keyEquivalent, val);
+
+ this.loadingCache.putAll(map);
Review Comment:
nit: this can be simplified
```suggestion
this.loadingCache.putAll(Collections.singletonMap(keyEquivalent, val));
```
```
##########
extensions-core/lookups-cached-single/src/main/java/org/apache/druid/server/lookup/LoadingLookup.java:
##########
@@ -73,15 +75,22 @@ public String apply(@Nullable final String key)
return null;
}
- final String presentVal;
- try {
- presentVal = loadingCache.get(keyEquivalent, new
ApplyCallable(keyEquivalent));
+ final String presentVal = this.loadingCache.getIfPresent(keyEquivalent);
+ if (presentVal != null) {
return NullHandling.emptyToNullIfNeeded(presentVal);
}
- catch (ExecutionException e) {
- LOGGER.debug("value not found for key [%s]", key);
+
+ String val = this.dataFetcher.fetch(keyEquivalent);
Review Comment:
```suggestion
final String val = this.dataFetcher.fetch(keyEquivalent);
```
##########
extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java:
##########
@@ -136,13 +157,16 @@ public void testGetCacheKey()
@Test
public void testSupportsAsMap()
{
- Assert.assertFalse(loadingLookup.supportsAsMap());
+ Assert.assertTrue(loadingLookup.supportsAsMap());
}
@Test
public void testAsMap()
{
- expectedException.expect(UnsupportedOperationException.class);
- loadingLookup.asMap();
+ Map.Entry<String, String> fetchedData = new
AbstractMap.SimpleEntry<>("dummy", "test");
Review Comment:
You could use `final Map<String, String> fetchedData = new HashMap<>();`.
Please include a couple more key-value pairs, one with null key and another
with null value. If someone changes the underlying implementation of `asMap()`,
this test will verify that the null cases are still handled.
##########
extensions-core/lookups-cached-single/src/test/java/org/apache/druid/server/lookup/LoadingLookupTest.java:
##########
@@ -136,13 +157,16 @@ public void testGetCacheKey()
@Test
public void testSupportsAsMap()
{
- Assert.assertFalse(loadingLookup.supportsAsMap());
+ Assert.assertTrue(loadingLookup.supportsAsMap());
}
@Test
public void testAsMap()
{
- expectedException.expect(UnsupportedOperationException.class);
- loadingLookup.asMap();
+ Map.Entry<String, String> fetchedData = new
AbstractMap.SimpleEntry<>("dummy", "test");
+
EasyMock.expect(dataFetcher.fetchAll()).andReturn(Collections.singletonList(fetchedData));
+ EasyMock.replay(dataFetcher);
+ Assert.assertEquals(loadingLookup.asMap().size(), 1);
Review Comment:
Since `asMap()` contract doesn't need any null handling transformation
(`emptyToNullIfNeeded()`), we can simply verify that the maps are equal:
```suggestion
Assert.assertEquals(fetchedData, loadingLookup.asMap())
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
