belugabehr commented on a change in pull request #2312:
URL: https://github.com/apache/hive/pull/2312#discussion_r639713247
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java
##########
@@ -416,58 +412,60 @@ public Path getCacheDirPath() {
/**
* Check if the cache contains an entry for the requested LookupInfo.
+ *
* @param request
- * @return The cached result if there is a match in the cache, or null if
no match is found.
+ * @return The cached result if there is a match in the cache, or null if no
+ * match is found.
+ * @throws NullPointerException if request is {@code null}
*/
- public CacheEntry lookup(LookupInfo request) {
- CacheEntry result = null;
+ public CacheEntry lookup(final LookupInfo request) {
+ Objects.requireNonNull(request);
- LOG.debug("QueryResultsCache lookup for query: {}", request.queryText);
+ LOG.debug("Cache lookup for query: {}", request.queryText);
+ CacheEntry result = null;
boolean foundPending = false;
- // Cannot entries while we currently hold read lock, so keep track of them
to delete later.
- Set<CacheEntry> entriesToRemove = new HashSet<CacheEntry>();
- Lock readLock = rwLock.readLock();
+ // Cannot modify entries while we currently hold read lock, so keep track
of
+ // them to delete later.
+ Set<CacheEntry> entriesToRemove = new HashSet<>();
+ cacheReadLock.lock();
try {
- // Note: ReentrantReadWriteLock deos not allow upgrading a read lock to
a write lock.
+ // Note: ReentrantReadWriteLock does not allow upgrading a read lock to
a write lock.
// Care must be taken while under read lock, to make sure we do not
perform any actions
// which attempt to take a write lock.
- readLock.lock();
- Set<CacheEntry> candidates = queryMap.get(request.queryText);
- if (candidates != null) {
- CacheEntry pendingResult = null;
- for (CacheEntry candidate : candidates) {
- if (entryMatches(request, candidate, entriesToRemove)) {
- CacheEntryStatus entryStatus = candidate.status;
- if (entryStatus == CacheEntryStatus.VALID) {
- result = candidate;
- break;
- } else if (entryStatus == CacheEntryStatus.PENDING &&
pendingResult == null) {
- pendingResult = candidate;
- }
+ Collection<CacheEntry> candidates = queryMap.get(request.queryText);
+ // Try to find valid entry, but settle for pending entry if that is all
+ // there is available
+ for (CacheEntry candidate : candidates) {
+ if (entryMatches(request, candidate, entriesToRemove)) {
+ CacheEntryStatus entryStatus = candidate.status;
+ if (entryStatus == CacheEntryStatus.VALID) {
+ result = candidate;
+ break;
+ }
+ if (entryStatus == CacheEntryStatus.PENDING) {
+ // Only accept first pending result
+ result = (result == null) ? candidate : result;
Review comment:
`queryMap` is now a Guava `Multimap` and therefore does not return null
values (it returns empty `Collection`). Otherwise just simplifying the code.
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
