Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes - #897
PHOENIX-6160 Simplifying concurrent mutation handling for global Indexes#897kadirozde wants to merge 2 commits into
Conversation
gjacoby126
left a comment
There was a problem hiding this 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.
| public Put getNextDataRowState(ImmutableBytesPtr rowKeyPtr) { | ||
| Pair<Put, Put> rowState = dataRowStates.get(rowKeyPtr); | ||
| if (rowState != null) { | ||
| return dataRowStates.get(rowKeyPtr).getSecond(); |
There was a problem hiding this comment.
nit: rowState.getSecond(). No need to pull out of the hashmap a second time.
There was a problem hiding this comment.
It was the intention and missed it. I will make the change
There was a problem hiding this 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)
| private String dataTableName; | ||
| private static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000; | ||
| private static final int DEFAULT_CONCURRENT_MUTATION_WAIT_DURATION_IN_MS = 1000; |
There was a problem hiding this comment.
I will change it to 100ms and let me know if you have a different suggestion
| // coprocessor calls. TODO: remove after HBASE-18127 when available | ||
| private static class BatchMutateContext { | ||
| private BatchMutatePhase currentPhase = BatchMutatePhase.PRE; |
There was a problem hiding this 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.
There was a problem hiding this 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.
| public void add() { | ||
| PendingRow(BatchMutateContext context) { | ||
| lastContext = context; |
There was a problem hiding this comment.
slightly cleaner if we can initialize the count to 1 in the constructor itself.
kadirozde
commented
Sep 29, 2020
Good suggestion. I will do some perf runs and update the Jira. |
kadirozde
commented
Sep 29, 2020
It seems I mistakenly closed this JIRA |
stoty
commented
Oct 12, 2020
💔 -1 overall
This message was automatically generated. |
gjacoby126
left a comment
There was a problem hiding this comment.
Thanks for the perf test, glad to see the numbers look good. +1.
No description provided.