Updated to handle partial page returns and query limiter feature#3637
Updated to handle partial page returns and query limiter feature#3637ivakegg wants to merge 8 commits into
Conversation
apmoriarty
left a comment
There was a problem hiding this comment.
Code Review
Summary
This PR makes four related changes:
- Introduces
isShortRunningQuery()onQueryLogicto optionally skip the async results thread. - Introduces
bypassQueryLimiter()onQueryLogicto skip ZooKeeper overhead for certain fast queries. - Flips the default for
RunningQuery.useResultsThreadfromfalse→true(all queries are now async by default). - Adds a partial-page-return mechanism:
QueryExpirationBeancan now detect a stuck-but-has-results query and callRunningQuery.attemptForcedPageReturn().
Bugs
1. Debug t.printStackTrace() left in production code — QueryExecutorBean.java ~L468
} catch (Throwable t) {
try {
if (null != qd.logic) {
t.printStackTrace(); // ← should not be here
qd.logic.close();
}
}This prints a raw stack trace to stdout (not to the logger). It must be removed before merge.
2. NullPointerException risk in CachedResultsBean.getQueryById() finally block
The refactoring introduced a try/finally to ensure query.setActiveCall(false) always runs:
try {
if (null == query) {
List<Query> queries = persister.findById(id);
if (null == queries || queries.isEmpty())
throw new NotFoundQueryException(...); // query still null here
if (queries.size() > 1)
throw new NotFoundQueryException(...); // query still null here
else {
// ... query gets set ...
}
} else { ... }
} finally {
query.setActiveCall(false); // NPE if query was never assigned
}If query is null on entry and persister.findById() throws, returns empty, or returns multiple results, the finally block throws NullPointerException on query.setActiveCall(false), masking the original exception. Fix: if (query != null) query.setActiveCall(false); in the finally.
3. currentThread field is not volatile
private Thread currentThread; // written in next(), read in QueryExpirationBean threadThis field is written at the start of next() on the query thread and read by QueryExpirationBean on a separate scheduler thread. Without volatile, the write may not be visible to the reading thread, so getCurrentThread().getStackTrace() could return stale or null state. Needs private volatile Thread currentThread;.
Design / Regression Concerns
4. Default useResultsThread changed from false → true is a major behavioral change
- private boolean useResultsThread = false;
+ private boolean useResultsThread = true;Previously, the async results thread was only used when isLongRunningQuery() or allowIntermediateEmptyPages was true. Now all queries use it by default unless isShortRunningQuery() returns true. Since no existing QueryLogic implementations override isShortRunningQuery() in this PR, all current queries silently switch to the async path.
The async thread introduces non-trivial overhead (thread handoff, queue synchronization, Object.wait/notify cycles). For short queries such as index lookups this could be a latency regression. The intent is documented, but the default-to-async change should be validated against performance benchmarks, or the default should remain false with an explicit opt-in.
5. isShortRunningQuery / bypassQueryLimiter in ShardQueryConfiguration appear unused
The PR adds fields, getters, setters, equals, hashCode, and copyFrom for these two flags to ShardQueryConfiguration. However, none of the callers in QueryExecutorBean, CachedResultsBean, or QueryExpirationBean read them from configuration — they call logic.isShortRunningQuery() and logic.bypassQueryLimiter() directly. If these config fields are intended to allow configuration-driven overrides, the wiring is missing; if not, they are dead code.
6. isShortRunningQuery() and bypassQueryLimiter() are non-default interface methods
// QueryLogic.java
boolean isShortRunningQuery();
boolean bypassQueryLimiter();These are added as abstract interface methods (not default). Any QueryLogic implementation that does not extend BaseQueryLogic will fail to compile. Adding default boolean isShortRunningQuery() { return false; } and default boolean bypassQueryLimiter() { return false; } would make this non-breaking for downstream consumers.
Minor Issues
7. Typo in comment — RunningQuery.java
// force us to not 8use asynchronous results thread is a short running queryShould be: // do not use asynchronous results thread for a short running query
8. Missing test coverage for attemptForcedPageReturn() and partial-page-return detection
QueryExpirationBean.isNextTooLong() has substantially new logic: it detects when currentPageCount > 0, logs the current thread stack trace, and calls query.attemptForcedPageReturn(). This new path has no unit tests. Given the complexity of the forcedReturn / hasNext / gotNext synchronization, this is high-risk untested code. At minimum there should be a test that verifies attemptForcedPageReturn() breaks out of the results loop in next().
Minor Positives
- The
try/finallyguards forsetActiveCall(false)throughoutQueryExecutorBeanandCachedResultsBeanare a sound correctness improvement — previously exceptions could leaveactiveCallin a bad state. - The
isIdleTooLong/isNextTooLongrefactoring (moving thehasActiveCall()guard inside each method) is cleaner. forcedReturnsynchronization using a dedicatedAtomicBooleanmonitor with bounds at the start and end ofnext()properly constrains thecurrentPageCountlifecycle.
flagging * Updated the running query to use the async results thread by default * Added a flag in the QueryLogic used to force synchronous running query * Added the capability for the query expiration bean to detect when a partial page should be returned and attempt to force it (includes detecting where the RunningQuery may be stuck if at all) * Added a flag in the Query Logic used to bypass the QueryLimiter mechanism (potentially for short lived queries such as UUID lookups)
7b03a3b to
2269083
Compare
flagging