You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/09/28 01:15:19 UTC
[GitHub] [phoenix] kadirozde opened a new pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
kadirozde opened a new pull request #897:
URL: https://github.com/apache/phoenix/pull/897
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r502979620
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -166,12 +175,36 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
private HashSet<ImmutableBytesPtr> rowsToLock = new HashSet<>();
// The current and next states of the data rows corresponding to the pending mutations
private HashMap<ImmutableBytesPtr, Pair<Put, Put>> dataRowStates;
- // Data table pending mutations
+ // The previous concurrent batch contexts
+ private HashMap<ImmutableBytesPtr, BatchMutateContext> lastConcurrentBatchContext = null;
+ // The latches of the threads waiting for this batch to complete
+ private List<CountDownLatch> waitList = null;
private Map<ImmutableBytesPtr, MultiMutation> multiMutationMap;
private BatchMutateContext(int clientVersion) {
this.clientVersion = clientVersion;
}
+
+ public BatchMutatePhase getCurrentPhase() {
+ return currentPhase;
+ }
+
+ public Put getNextDataRowState(ImmutableBytesPtr rowKeyPtr) {
+ Pair<Put, Put> rowState = dataRowStates.get(rowKeyPtr);
+ if (rowState != null) {
+ return dataRowStates.get(rowKeyPtr).getSecond();
Review comment:
> > Algorithm makes sense, it's a nice simplification, and just had a few nits on the code. My bigger concern is the potential increase in tail latency on writes to hot rows in write latency-sensitive applications. Would be good to get some perf numbers.
>
> Good suggestion. I will do some perf runs and update the Jira.
@gjacoby126, I have updated the design doc with the performance testing results ( https://docs.google.com/document/d/12H_MwsPtyM0ORiBHclBpBLZWtm4zpY_cc5y_pwtgMUk/edit#heading=h.yt8378ps0k6e)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497110937
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -150,9 +154,14 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
failDataTableUpdatesForTesting = fail;
}
+ public enum BatchMutatePhase {
+ PRE, POST, FAILED
+ }
// Hack to get around not being able to save any state between
// coprocessor calls. TODO: remove after HBASE-18127 when available
+
private static class BatchMutateContext {
+ private BatchMutatePhase currentPhase = BatchMutatePhase.PRE;
Review comment:
Great question! The concurrent batch of mutations is a set such that every pair of batches in this set has at least one common row. Since a BatchMutateContext object of a batch is modified only after the row locks for all the rows that are mutated by this batch are acquired, there can be only one thread can acquire the locks for its batch and safely access all the batch contexts in the set of concurrent batches. I will add this to doc and comment it on the code. Please note that the row state is also accessed. So, atomic variables are not necessary. Also, making currentPhase atomic and leaving the rest as it is will give a wrong impression.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497110937
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -150,9 +154,14 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
failDataTableUpdatesForTesting = fail;
}
+ public enum BatchMutatePhase {
+ PRE, POST, FAILED
+ }
// Hack to get around not being able to save any state between
// coprocessor calls. TODO: remove after HBASE-18127 when available
+
private static class BatchMutateContext {
+ private BatchMutatePhase currentPhase = BatchMutatePhase.PRE;
Review comment:
Great question! The concurrent batch of mutations is a set such that every pair of batches in this set has at least one common row. Since a BatchMutateContext object of a batch is modified only after the row locks for all the rows that are mutated by this batch are acquired, there can be only one thread can acquire the locks for its batch and safely access all the batch contexts in the set of concurrent batches. I will add this to doc and comment it on the code. Please note that the row state is also accessed. So, atomic variables are not necessary. Also, making currentPhase atomic and leave the rest as it is will give the wrong impression.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r496949729
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -166,12 +175,36 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
private HashSet<ImmutableBytesPtr> rowsToLock = new HashSet<>();
// The current and next states of the data rows corresponding to the pending mutations
private HashMap<ImmutableBytesPtr, Pair<Put, Put>> dataRowStates;
- // Data table pending mutations
+ // The previous concurrent batch contexts
+ private HashMap<ImmutableBytesPtr, BatchMutateContext> lastConcurrentBatchContext = null;
+ // The latches of the threads waiting for this batch to complete
+ private List<CountDownLatch> waitList = null;
private Map<ImmutableBytesPtr, MultiMutation> multiMutationMap;
private BatchMutateContext(int clientVersion) {
this.clientVersion = clientVersion;
}
+
+ public BatchMutatePhase getCurrentPhase() {
+ return currentPhase;
+ }
+
+ public Put getNextDataRowState(ImmutableBytesPtr rowKeyPtr) {
+ Pair<Put, Put> rowState = dataRowStates.get(rowKeyPtr);
+ if (rowState != null) {
+ return dataRowStates.get(rowKeyPtr).getSecond();
Review comment:
nit: rowState.getSecond(). No need to pull out of the hashmap a second time.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -211,9 +244,11 @@ private BatchMutateContext(int clientVersion) {
private long slowIndexPrepareThreshold;
private long slowPreIncrementThreshold;
private int rowLockWaitDuration;
+ private int concurrentMutationWaitDuration;
private String dataTableName;
private static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000;
+ private static final int DEFAULT_CONCURRENT_MUTATION_WAIT_DURATION_IN_MS = 1000;
Review comment:
1s seems long
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -150,9 +154,14 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
failDataTableUpdatesForTesting = fail;
}
+ public enum BatchMutatePhase {
+ PRE, POST, FAILED
+ }
// Hack to get around not being able to save any state between
// coprocessor calls. TODO: remove after HBASE-18127 when available
+
private static class BatchMutateContext {
+ private BatchMutatePhase currentPhase = BatchMutatePhase.PRE;
Review comment:
This variable gets accessed from multiple threads -- should it be atomic? Since only one thread will write to it at once, it would currently only occasionally prevent having to go through the wait loop more than times than necessary. Might also prevent correctness issues in the future if these assumptions change.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497111243
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -117,12 +118,15 @@
* Class to represent pending data table rows
*/
private static class PendingRow {
- private boolean concurrent = false;
private long count = 1;
+ private BatchMutateContext lastContext;
- public void add() {
+ PendingRow(BatchMutateContext context) {
+ lastContext = context;
Review comment:
Will do that
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde closed pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde closed pull request #897:
URL: https://github.com/apache/phoenix/pull/897
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497252684
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -150,9 +154,14 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
failDataTableUpdatesForTesting = fail;
}
+ public enum BatchMutatePhase {
+ PRE, POST, FAILED
+ }
// Hack to get around not being able to save any state between
// coprocessor calls. TODO: remove after HBASE-18127 when available
+
private static class BatchMutateContext {
+ private BatchMutatePhase currentPhase = BatchMutatePhase.PRE;
Review comment:
Makes sense, thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde closed pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde closed pull request #897:
URL: https://github.com/apache/phoenix/pull/897
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r496987773
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -117,12 +118,15 @@
* Class to represent pending data table rows
*/
private static class PendingRow {
- private boolean concurrent = false;
private long count = 1;
+ private BatchMutateContext lastContext;
- public void add() {
+ PendingRow(BatchMutateContext context) {
+ lastContext = context;
Review comment:
slightly cleaner if we can initialize the count to 1 in the constructor itself.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on pull request #897:
URL: https://github.com/apache/phoenix/pull/897#issuecomment-701034898
It seems I mistakenly closed this JIRA
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497099335
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -166,12 +175,36 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
private HashSet<ImmutableBytesPtr> rowsToLock = new HashSet<>();
// The current and next states of the data rows corresponding to the pending mutations
private HashMap<ImmutableBytesPtr, Pair<Put, Put>> dataRowStates;
- // Data table pending mutations
+ // The previous concurrent batch contexts
+ private HashMap<ImmutableBytesPtr, BatchMutateContext> lastConcurrentBatchContext = null;
+ // The latches of the threads waiting for this batch to complete
+ private List<CountDownLatch> waitList = null;
private Map<ImmutableBytesPtr, MultiMutation> multiMutationMap;
private BatchMutateContext(int clientVersion) {
this.clientVersion = clientVersion;
}
+
+ public BatchMutatePhase getCurrentPhase() {
+ return currentPhase;
+ }
+
+ public Put getNextDataRowState(ImmutableBytesPtr rowKeyPtr) {
+ Pair<Put, Put> rowState = dataRowStates.get(rowKeyPtr);
+ if (rowState != null) {
+ return dataRowStates.get(rowKeyPtr).getSecond();
Review comment:
It was the intention and missed it. I will make the change
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497110937
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -150,9 +154,14 @@ public static void setFailDataTableUpdatesForTesting(boolean fail) {
failDataTableUpdatesForTesting = fail;
}
+ public enum BatchMutatePhase {
+ PRE, POST, FAILED
+ }
// Hack to get around not being able to save any state between
// coprocessor calls. TODO: remove after HBASE-18127 when available
+
private static class BatchMutateContext {
+ private BatchMutatePhase currentPhase = BatchMutatePhase.PRE;
Review comment:
Great question! The concurrent batch of mutations is a set such that every pair of batches in this set has at least one common row. Since a BatchMutateContext object of a batch is modified only after the row locks for all the rows that are mutated by this batch are acquired, there can be only one thread can acquire the locks for its batch and safely access all the batch contexts in the set of concurrent batches. I will add this to doc and comment it on the code. Please note that the row state is also accessed. So, atomic variables are not necessary. Also, making currentPhase atomic and leaving the rest as it is will give the wrong impression.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on pull request #897:
URL: https://github.com/apache/phoenix/pull/897#issuecomment-701033822
> Algorithm makes sense, it's a nice simplification, and just had a few nits on the code. My bigger concern is the potential increase in tail latency on writes to hot rows in write latency-sensitive applications. Would be good to get some perf numbers.
Good suggestion. I will do some perf runs and update the Jira.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] kadirozde commented on a change in pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
kadirozde commented on a change in pull request #897:
URL: https://github.com/apache/phoenix/pull/897#discussion_r497099967
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
##########
@@ -211,9 +244,11 @@ private BatchMutateContext(int clientVersion) {
private long slowIndexPrepareThreshold;
private long slowPreIncrementThreshold;
private int rowLockWaitDuration;
+ private int concurrentMutationWaitDuration;
private String dataTableName;
private static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000;
+ private static final int DEFAULT_CONCURRENT_MUTATION_WAIT_DURATION_IN_MS = 1000;
Review comment:
I will change it to 100ms and let me know if you have a different suggestion
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #897: PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #897:
URL: https://github.com/apache/phoenix/pull/897#issuecomment-706836515
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 6m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 31m 39s | 4.x passed |
| +1 :green_heart: | compile | 1m 0s | 4.x passed |
| +1 :green_heart: | checkstyle | 0m 37s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 47s | 4.x passed |
| +0 :ok: | spotbugs | 3m 6s | phoenix-core in 4.x has 957 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 29m 24s | the patch passed |
| +1 :green_heart: | compile | 0m 59s | the patch passed |
| +1 :green_heart: | javac | 0m 59s | the patch passed |
| -1 :x: | checkstyle | 0m 37s | phoenix-core: The patch generated 64 new + 375 unchanged - 44 fixed = 439 total (was 419) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 47s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 19s | phoenix-core generated 0 new + 956 unchanged - 1 fixed = 956 total (was 957) |
||| _ Other Tests _ |
| -1 :x: | unit | 174m 40s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 31s | The patch does not generate ASF License warnings. |
| | | 256m 29s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.index.IndexMetadataIT |
| | phoenix.end2end.IndexExtendedIT |
| | phoenix.end2end.ConnectionUtilIT |
| | phoenix.end2end.SkipScanQueryIT |
| | phoenix.end2end.index.IndexCoprocIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-897/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/897 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux fb953d384f5a 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / fc3d25c |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-897/3/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-897/3/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-897/3/testReport/ |
| Max. process+thread count | 6238 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-897/3/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
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:
users@infra.apache.org