You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by vincentpoon <gi...@git.apache.org> on 2018/01/26 01:52:34 UTC
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
GitHub user vincentpoon opened a pull request:
https://github.com/apache/phoenix/pull/290
PHOENIX-4130 Avoid server retries for mutable indexes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vincentpoon/phoenix PHOENIX-4130
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/290.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #290
----
commit cb86a5b6a42e3c3a62d75851499a1460b241d623
Author: Vincent Poon <vi...@...>
Date: 2018-01-26T01:25:09Z
PHOENIX-4130 Avoid server retries for mutable indexes
----
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164181998
--- 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 --
Since this is running client-side, the EnvironmentEdgeManager.currentTimeMillis() would be a client timestamp and the indexDisableTimestamp is a server-side timestamp so you can't reliably compare them directly. Instead, keep a bit more state client-side by storing the timestamp at which the index write failure occurred in the index TableRef (which isn't shared across multiple threads/connections). Then you can compare that timestamp here with the current time.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164205028
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
A simpler solution would be to only set INDEX_WRITER_RPC_RETRIES_NUMBER if it's not set (Configuration.setIfNotSet()). That way an installation could set the config param to 1 until they know that clients have been upgraded at which point they could remove the setting.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164264253
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/index/PartialIndexRebuilderIT.java ---
@@ -1075,6 +1111,12 @@ public void testIndexFailureWithinRSDisablesIndex() throws Throwable {
public static class WriteFailingRegionObserver extends SimpleRegionObserver {
@Override
public void postBatchMutate(ObserverContext<RegionCoprocessorEnvironment> c, MiniBatchOperationInProgress<Mutation> miniBatchOp) throws IOException {
+ // we need to advance the clock, since the index retry logic (copied from HBase) has a time component
+ EnvironmentEdge delegate = EnvironmentEdgeManager.getDelegate();
+ if (delegate instanceof MyClock) {
+ MyClock myClock = (MyClock) delegate;
+ myClock.time += 1000;
+ }
--- End diff --
Do we still need this since you're using TableRef.getCurrentTime()? If yes, that's ok.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164264280
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java ---
@@ -312,6 +320,15 @@ 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, long currentTimestamp, long indexDisableTimestamp) {
--- End diff --
You could just make addPlan and this method non static and then you could have a member variable for indexPendingDisableThreshold. Might be a little bit cleaner that way IMHO.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164577121
--- 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 --
@JamesRTaylor So found out the problem is that for index tables that are disabled, in MetaDataEndpointImpl we use the the indexDisableTimestamp for the mutationTime. There's this comment:
" // Freeze time for table at min non-zero value of INDEX_DISABLE_TIMESTAMP
// This will keep the table consistent with index as the table has had one more
// batch applied to it."
Is that still required? I think I have to find another way to get the server time because of this.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164551154
--- 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 --
If you're rerunning the query, the current time should change. You should have a new TableRef for the new query (the old one won't change). The timestamp should come from the client-side cache and should be updated in MetaDataClient.updateCache() here:
// if we aren't adding the table, we still need to update the resolved time of the table
connection.updateResolvedTimestamp(table, resolvedTime);
The updateCache call should be made by FromCompiler.BaseColumnResolver.createTableRef() and the timestamp in the TableRef should be set based on the one in MetaDataMutationResult.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164180172
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataRegionObserver.java ---
@@ -379,6 +388,12 @@ public void run() {
} else if (indexState == PIndexState.PENDING_ACTIVE) {
IndexUtil.updateIndexState(conn, indexTableFullName, PIndexState.ACTIVE, null);
continue; // Must wait until clients start to do index maintenance again
+ } else if (indexState == PIndexState.PENDING_DISABLE) {
+ if (elapsedSinceDisable > pendingDisableThreshold) {
+ // too long in PENDING_DISABLE - client didn't disable the index, so we do it here
+ IndexUtil.updateIndexState(conn, indexTableFullName, PIndexState.DISABLE, null);
--- End diff --
This will put the index into a permanently disabled state requiring manual intervention. I think we should not pass in null for the indexDisableTimestamp here, but pass in the current value instead. That'll cause the index to be partially rebuilt (as long as it's not older that the threshold to permanently disable it).
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164208947
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
Current installations may not have any property configured and are just using the default of 11. Post-upgrade server side, they would again be using the default, which has changed to 1. So I think we would still need to check the client version?
Unless you're saying the onus should be on the operator to set configs based on the version being used, but that seems more confusing.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164202405
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataRegionObserver.java ---
@@ -379,6 +388,12 @@ public void run() {
} else if (indexState == PIndexState.PENDING_ACTIVE) {
IndexUtil.updateIndexState(conn, indexTableFullName, PIndexState.ACTIVE, null);
continue; // Must wait until clients start to do index maintenance again
+ } else if (indexState == PIndexState.PENDING_DISABLE) {
+ if (elapsedSinceDisable > pendingDisableThreshold) {
+ // too long in PENDING_DISABLE - client didn't disable the index, so we do it here
+ IndexUtil.updateIndexState(conn, indexTableFullName, PIndexState.DISABLE, null);
--- End diff --
I think passing null leaves the indexDisableTimestamp untouched, but since we have the timestamp I'll pass it in anyways to make it clearer.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164218981
--- 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 --
Sounds fine
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164184711
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
Maybe the easiest way to do that is in Indexer.start() you could do a getTable call on SYSTEM.CATALOG and check the table timestamp - if it's been upgraded to 4.14, use 1, otherwise use 11. Though it'd be an RPC, we'd know that SYSTEM.CATALOG would be cached in our server-side cache so it'd be relatively cheap.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164220749
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
I was thinking it'd be ok if the onus was on the operator. I think we're in the minority in having a mix of old/new clients with zero downtime. If you think it's worth checking the client version (or checking if upgraded yet) and can come up with a reasonable way of doing that, that's fine too. Not sure it's worth the effort, though. Here's some ideas:
* add client version to IndexMaintainer (which is protobuf-ed, so it'd be doable). Then use the "correct" IndexBuilder (or config?) based on the client version pulled out of the IndexMaintainer. If you have separate IndexBuilder instances, then you'd get duplicate thread pools to do the writing too which isn't ideal. Maybe within the parallel writer you could use the correct config?
* less dynamic, but check the timestamp of system.catalog to know what version you're at. Somewhat scary to have an RPC in the start path of the coprocessor, though, as if system.catalog RS can't be reached, then you're kind of screwed (but many you are already anyway).
* least dynamic - put onus on operator and change to setIfUnset.
---
[GitHub] phoenix issue #290: PHOENIX-4130 Avoid server retries for mutable indexes
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on the issue:
https://github.com/apache/phoenix/pull/290
I have a test in PartialIndexRebuilderIt#testPendingDisable() which tests for when an index stays in PENDING_DISABLE too long. It tests that the client queryOptimizer does not use the index, and that the rebuilder switches the index to DISABLE.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164537544
--- 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 --
@JamesRTaylor I'm seeing the following issue with using indexTableRef.getCurrentTime(). In my test, if set the index to PENDING_DISABLE. I then perform a query, which uses the index as expected. If I then advance the EnvironmentEdgeManager clock to beyond the threshold, then perform the query again, it still uses the index because indexTableRef.getCurrentTime() hasn't changed. Is this expected?
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164182566
--- 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
+ .getConnection().getQueryServices().getConfiguration()
+ .getLong(QueryServices.INDEX_PENDING_DISABLE_THRESHOLD,
+ QueryServicesOptions.DEFAULT_INDEX_PENDING_DISABLE_THRESHOLD);
--- End diff --
Use .getConnection().getQueryServices().getProps() instead and add a member variable to QueryOptimizer for indexPendingDisableThreshold. The ReadOnlyProps should be used client-side as it's an immutable, read-only copy of Configuration and prevents lock contention that we were seeing on the Configuration object when there are many clients on a single JVM.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164211639
--- 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 --
I wanted to add a member variable to hold the threshold, but these methods are static, and I wasn't sure if adding a mutable static was advisable. The call to isUnderPendingDIsableThreshold() is the last thing in the if-statement, so this only gets called in a PENDING_DISABLE scenario. I'll add the optimization to use ReadOnlyProps, and use the client-side timestamp
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164264235
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ConcurrentMutationsIT.java ---
@@ -211,6 +211,8 @@ public void run() {
t2.start();
doneSignal.await(60, TimeUnit.SECONDS);
+ TestUtil.dumpTable(conn.unwrap(PhoenixConnection.class).getQueryServices().getTable(Bytes.toBytes(tableName)));
+ TestUtil.dumpTable(conn.unwrap(PhoenixConnection.class).getQueryServices().getTable(Bytes.toBytes(indexName)));
--- End diff --
Minor nit: remove these dumpTable calls.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164235494
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
Good point.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164236923
--- 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 --
Good point. So instead of using EnvironmentEdgeManager.currentTimeMillis(), use indexTableRef.getCurrentTime(). That'll be the server-side timestamp when the index table was resolved during query compilation. The only caveat is that when UPDATE_CACHE_FREQUENCY is set, we wouldn't ping the server so we wouldn't have a timestamp. There are numerous issues related to this already, though, so I think this would be fine. For example, if we don't ping the server, we wouldn't know that the index is in a PENDING_DISABLE state either. We already have a JIRA on this, so we can tackle these issues down-the-road (and we're not using UPDATE_CACHE_FREQUENCY yet).
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
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.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164230652
--- 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 --
@JamesRTaylor
> we should store the client timestamp when we first received the exception
What about cases where the client never got the exception, though? That's actually the case that this code is meant to address - if for some reason a client is writing and then goes away before receiving the IndexWriteException, we want new/other client threads to not use the index if the index is in PENDING_DISABLE for too long.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164581120
--- 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 --
The timestamp freeze should only happen when QueryServices.INDEX_FAILURE_BLOCK_WRITE is true. It's false by default. There might be a bug here.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164226421
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
I don't think we need any code change if we're relying on the operator. As it is, we check the config INDEX_WRITER_RPC_RETRIES_NUMBER, and only if it's not there, we use the DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER, which is now 1. So we assume the operator will have the property set to 11 until all clients are upgraded, at which point the remove the property and it uses the default of 1.
---
[GitHub] phoenix pull request #290: PHOENIX-4130 Avoid server retries for mutable ind...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/290#discussion_r164180657
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/write/IndexWriterUtils.java ---
@@ -70,13 +70,13 @@
public static final String HTABLE_KEEP_ALIVE_KEY = "hbase.htable.threads.keepalivetime";
public static final String INDEX_WRITER_RPC_RETRIES_NUMBER = "phoenix.index.writes.rpc.retries.number";
- /**
- * Based on the logic in HBase's AsyncProcess, a default of 11 retries with a pause of 100ms
- * approximates 48 sec total retry time (factoring in backoffs). The total time should be less
- * than HBase's rpc timeout (default of 60 sec) or else the client will retry before receiving
- * the response
- */
- public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 11;
+ /**
+ * Retry server-server index write rpc only once, and let the client retry the data write
+ * instead to avoid typing up the handler
+ */
+ // note in HBase 2+, numTries = numRetries + 1
+ // in prior versions, numTries = numRetries
+ public static final int DEFAULT_INDEX_WRITER_RPC_RETRIES_NUMBER = 1;
--- End diff --
We need to conditionally leave it at 11 versus use 1 based on the client version. Old clients wouldn't have the retry logic, so we'd need to continue retrying on the server.
---