Github user JamesRTaylor commented on a diff in the pull request:

    https://github.com/apache/phoenix/pull/290#discussion_r164171049
  
    --- Diff: 
phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java ---
    @@ -312,6 +317,16 @@ private static QueryPlan addPlan(PhoenixStatement 
statement, SelectStatement sel
             return null;
         }
     
    +    // returns true if we can still use the index
    +    // retuns false if we've been in PENDING_DISABLE too long - index 
should be considered disabled
    +    private static boolean isUnderPendingDisableThreshold(PhoenixStatement 
statement, PTable indexTable) {
    +        return EnvironmentEdgeManager
    +                .currentTimeMillis() - 
indexTable.getIndexDisableTimestamp() <= statement
    --- End diff --
    
    This runs on the client-side, but the INDEX_DISABLED_TIMESTAMP was set on 
the server-side, so we can't necessarily reliably compare them. Instead, we 
should store the client timestamp when we first received the exception and 
compare the client-side current time with that. Perhaps a good place to store 
this pendingDisableInitialTimestamp would be on the index TableRef which would 
not be shared across multiple threads.
    
    Also, minor nit: whenever possible (if on client-side), use 
.getConnection().getQueryServices().getProps() instead of going through 
Configuration. We created this ReadOnlyProps which is essentially a copy of 
Configuration but as an read-only, immutable map because we were seeing 
contention on the locking done by Configuration (which is not necessary because 
it's used in an immutable way). You can also add an 
indexPendingDisableThreshold as a member variable to QueryOptimizer.


---

Reply via email to