-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add result level caching to Brokers #5028
Changes from 1 commit
ba78816
7cb33cd
83ee76e
24a7595
d2861b9
130ee63
efeb2b2
08b4feb
3639c0a
e1d9175
c805b92
d81d81c
d738fbc
7e3492c
fc69327
0d409ea
04878cd
ead2dd9
cb12828
8abf5ac
d00bb28
6bd296f
53e8056
d4b823d
6586b5c
cb107f9
8dd436d
34d0128
07eb46d
9d05221
5a9dc8c
46da9e5
bcddabf
6c42e2c
d51c21e
b8547bb
96bbf23
28a7a0e
9dec1cd
b5b7992
edd79c0
7f2d48d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,9 +41,7 @@ | |
import java.io.ByteArrayOutputStream; | ||
import java.io.IOException; | ||
import java.nio.ByteBuffer; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Map; | ||
|
||
public class ResultLevelCachingQueryRunner<T> implements QueryRunner<T> | ||
|
@@ -108,18 +106,21 @@ public Sequence<T> run(QueryPlus queryPlus, Map responseContext) | |
cacheKeyStr, | ||
newResultSetId | ||
); | ||
|
||
if (resultLevelCachePopulator == null) { | ||
return resultFromClient; | ||
} | ||
final Function<T, Object> cacheFn = strategy.prepareForCache(true); | ||
|
||
return Sequences.wrap(Sequences.map( | ||
resultFromClient, | ||
new Function<T, T>() | ||
{ | ||
@Override | ||
public T apply(T input) | ||
{ | ||
cacheResultEntry(resultLevelCachePopulator, input); | ||
if (resultLevelCachePopulator.isShouldPopulate()) { | ||
resultLevelCachePopulator.cacheResultEntry(resultLevelCachePopulator, input, cacheFn); | ||
} | ||
return input; | ||
} | ||
} | ||
|
@@ -128,14 +129,22 @@ public T apply(T input) | |
@Override | ||
public void after(boolean isDone, Throwable thrown) throws Exception | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
Preconditions.checkNotNull(resultLevelCachePopulator, "ResultLevelCachePopulator cannot be null during cache population"); | ||
Preconditions.checkNotNull( | ||
resultLevelCachePopulator, | ||
"ResultLevelCachePopulator cannot be null during cache population" | ||
); | ||
if (thrown != null) { | ||
log.error("Error while preparing for result level caching for query %s ", query.getId()); | ||
} else { | ||
log.error( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (minor) why not put the error as a parameter to |
||
"Error while preparing for result level caching for query %s with error %s ", | ||
query.getId(), | ||
thrown.getMessage() | ||
); | ||
} else if (resultLevelCachePopulator.isShouldPopulate()) { | ||
// The resultset identifier and its length is cached along with the resultset | ||
resultLevelCachePopulator.populateResults(newResultSetId); | ||
resultLevelCachePopulator.populateResults(); | ||
log.debug("Cache population complete for query %s", query.getId()); | ||
} | ||
resultLevelCachePopulator.cacheObjectStream.close(); | ||
} | ||
}); | ||
} | ||
|
@@ -147,17 +156,6 @@ public void after(boolean isDone, Throwable thrown) throws Exception | |
} | ||
} | ||
|
||
private T cacheResultEntry( | ||
ResultLevelCachePopulator resultLevelCachePopulator, | ||
T resultEntry | ||
) | ||
{ | ||
final Function<T, Object> cacheFn = strategy.prepareForCache(true); | ||
resultLevelCachePopulator.cacheObjects | ||
.add(cacheFn.apply(resultEntry)); | ||
return resultEntry; | ||
} | ||
|
||
private byte[] fetchResultsFromResultLevelCache( | ||
final String queryCacheKey | ||
) | ||
|
@@ -216,12 +214,25 @@ private ResultLevelCachePopulator createResultLevelCachePopulator( | |
) | ||
{ | ||
if (resultSetId != null && populateResultCache) { | ||
return new ResultLevelCachePopulator( | ||
ResultLevelCachePopulator resultLevelCachePopulator = new ResultLevelCachePopulator( | ||
cache, | ||
objectMapper, | ||
ResultLevelCacheUtil.computeResultLevelCacheKey(cacheKeyStr), | ||
cacheConfig | ||
cacheConfig, | ||
true | ||
); | ||
try { | ||
// Save the resultSetId and its length | ||
resultLevelCachePopulator.cacheObjectStream.write(ByteBuffer.allocate(Integer.BYTES) | ||
.putInt(resultSetId.length()) | ||
.array()); | ||
resultLevelCachePopulator.cacheObjectStream.write(StringUtils.toUtf8(resultSetId)); | ||
} | ||
catch (IOException ioe) { | ||
log.error("Failed to write cached values for query %s", query.getId()); | ||
return null; | ||
} | ||
return resultLevelCachePopulator; | ||
} else { | ||
return null; | ||
} | ||
|
@@ -232,60 +243,59 @@ public class ResultLevelCachePopulator | |
private final Cache cache; | ||
private final ObjectMapper mapper; | ||
private final Cache.NamedKey key; | ||
private final List<Object> cacheObjects = new ArrayList<>(); | ||
private final CacheConfig cacheConfig; | ||
private final ByteArrayOutputStream cacheObjectStream = new ByteArrayOutputStream(); | ||
|
||
public boolean isShouldPopulate() | ||
{ | ||
return shouldPopulate; | ||
} | ||
|
||
private boolean shouldPopulate; | ||
|
||
private ResultLevelCachePopulator( | ||
Cache cache, | ||
ObjectMapper mapper, | ||
Cache.NamedKey key, | ||
CacheConfig cacheConfig | ||
CacheConfig cacheConfig, | ||
boolean shouldPopulate | ||
) | ||
{ | ||
this.cache = cache; | ||
this.mapper = mapper; | ||
this.key = key; | ||
this.cacheConfig = cacheConfig; | ||
this.shouldPopulate = shouldPopulate; | ||
} | ||
|
||
public void populateResults(String resultSetIdentifier) | ||
private void cacheResultEntry( | ||
ResultLevelCachePopulator resultLevelCachePopulator, | ||
T resultEntry, | ||
Function<T, Object> cacheFn | ||
) | ||
{ | ||
ByteArrayOutputStream bytes = new ByteArrayOutputStream(); | ||
try { | ||
// Save the resultSetId and its length | ||
bytes.write(ByteBuffer.allocate(Integer.BYTES).putInt(resultSetIdentifier.length()).array()); | ||
bytes.write(StringUtils.toUtf8(resultSetIdentifier)); | ||
byte[] resultBytes = fetchResultBytes(bytes, cacheConfig.getCacheBulkMergeLimit()); | ||
if (resultBytes != null) { | ||
ResultLevelCacheUtil.populate( | ||
cache, | ||
key, | ||
resultBytes | ||
); | ||
} | ||
// Help out GC by making sure all references are gone | ||
cacheObjects.clear(); | ||
|
||
int cacheLimit = cacheConfig.getResultLevelCacheLimit(); | ||
if (cacheLimit > 0 && resultLevelCachePopulator.cacheObjectStream.size() > cacheLimit) { | ||
shouldPopulate = false; | ||
return; | ||
} | ||
catch (IOException ioe) { | ||
log.error("Failed to write cached values for query %s", query.getId()); | ||
try (JsonGenerator gen = mapper.getFactory().createGenerator(resultLevelCachePopulator.cacheObjectStream)) { | ||
gen.writeObject(cacheFn.apply(resultEntry)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if last element in sequence took you over the size limit, shouldPopulate would still stay true resulting in it getting stored even if it crossed the size limit. also, as soon as we cross the size limit... we should discard the data stored in cacheObjectStream because we know it is not going to be used and let it be GC'd . |
||
} | ||
catch (IOException ex) { | ||
log.error("Failed to retrieve entry to be cached. Result Level caching will not be performed!"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should probably log error here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aka, include the exception in the error parameters for the logger call |
||
shouldPopulate = false; | ||
} | ||
} | ||
|
||
private byte[] fetchResultBytes(ByteArrayOutputStream resultStream, int cacheLimit) | ||
public void populateResults() | ||
{ | ||
for (Object cacheObj : cacheObjects) { | ||
try (JsonGenerator gen = mapper.getFactory().createGenerator(resultStream)) { | ||
gen.writeObject(cacheObj); | ||
if (cacheLimit > 0 && resultStream.size() > cacheLimit) { | ||
return null; | ||
} | ||
} | ||
catch (IOException ex) { | ||
log.error("Failed to retrieve entry to be cached. Result Level caching will not be performed!"); | ||
return null; | ||
} | ||
} | ||
return resultStream.toByteArray(); | ||
ResultLevelCacheUtil.populate( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it might be cleaner to do the check whether we crossed limit at this point. I know its done externally but may be clearer to do that here. |
||
cache, | ||
key, | ||
cacheObjectStream.toByteArray() | ||
); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are doing this check for every row in the result set whereas it should only be done once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nmnd, i see its checking whether we've crossed the size limit