[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16721139#comment-16721139
]
Hudson commented on HBASE-17924:
Results for branch branch-1.3
[build #576 on
builds.a.o|https://builds.apache.org/job/HBase%20Nightly/job/branch-1.3/576/]:
(x) *{color:red}-1 overall{color}*
details (if available):
(/) {color:green}+1 general checks{color}
-- For more information [see general
report|https://builds.apache.org/job/HBase%20Nightly/job/branch-1.3/576//General_Nightly_Build_Report/]
(/) {color:green}+1 jdk7 checks{color}
-- For more information [see jdk7
report|https://builds.apache.org/job/HBase%20Nightly/job/branch-1.3/576//JDK7_Nightly_Build_Report/]
(x) {color:red}-1 jdk8 hadoop2 checks{color}
-- For more information [see jdk8 (hadoop2)
report|https://builds.apache.org/job/HBase%20Nightly/job/branch-1.3/576//JDK8_Nightly_Build_Report_(Hadoop2)/]
(/) {color:green}+1 source release artifact{color}
-- See build output for details.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 1.1.8, 2.0.0
>Reporter: Andrew Purtell
>Assignee: Allan Yang
>Priority: Major
> Fix For: 1.4.0, 1.3.3, 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16720752#comment-16720752
]
Hudson commented on HBASE-17924:
SUCCESS: Integrated in Jenkins build HBase-1.3-IT #509 (See
[https://builds.apache.org/job/HBase-1.3-IT/509/])
HBASE-17924 Consider sorting the row order when processing multi() ops
(apurtell: rev 4e4756e68887e3a1d77e4483f52f431b441611a5)
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
* (edit) hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALSplitter.java
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 1.1.8, 2.0.0
>Reporter: Andrew Purtell
>Assignee: Allan Yang
>Priority: Major
> Fix For: 1.4.0, 1.3.3, 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16394871#comment-16394871
]
mimani commented on HBASE-17924:
Forgive my ignorance, Can you point me on how to apply this patch on
hbase-1.4.2 ?
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
>Priority: Major
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16054860#comment-16054860
]
Jerry He commented on HBASE-17924:
--
Thanks for the pointer. I will be reading thru the latest goodies on
HBASE-18144.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16053479#comment-16053479
]
stack commented on HBASE-17924:
---
To close the loop here, the [~jerryhe] question above is getting an answer over
on the tail of HBASE-18144 from [~allan163]
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16020833#comment-16020833
]
Hudson commented on HBASE-17924:
FAILURE: Integrated in Jenkins build HBase-HBASE-14614 #244 (See
[https://builds.apache.org/job/HBase-HBASE-14614/244/])
HBASE-17924 Consider sorting the row order when processing multi() ops
(apurtell: rev 959deb0e5c7c43c2cda6fe4b5c4d46ed80d3d8e6)
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
* (edit) hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALSplitter.java
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16019135#comment-16019135
]
stack commented on HBASE-17924:
---
I like [~jerryhe]'s question. Not sure how sort makes a difference
("...conflicts faster..." implies concurrent threads are moving through the
batch in lock-step); and if we are going to sort batches, would be good to do
it across the board so we could lean on batches being sorted (say, skip taking
lock if second batch on a row); i.e. +1 on the semantic improvement. I'm in
this section at the mo. It is looking like our reeentrant read/write lock, the
basis of our current row locking, goes to hell if there are more than a few
read-then-modify operations in the mix (i.e. a bunch of increments on a row
also taking mutations). Thanks.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16018986#comment-16018986
]
Andrew Purtell commented on HBASE-17924:
We acquire row locks in order. Sorting the row order first means we find
conflicts faster (the first common row in sort order). The performance gain may
be hard to determine but there is also a semantics improvement. No matter what
clients are doing row lock acquisition will be in a well known order. This
latter point was our motivation over in the Phoenix project for suggesting this
change.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16018966#comment-16018966
]
Jerry He commented on HBASE-17924:
--
Thinking a little more. It is not clear how the ordering will help multi
performance.
The locks for doMiniBatchMutate() are read locks for put and delete.
Say one batch has (a, c, b), another thread has (b, a, c). Sorting the rows
or not, there is no conflict anyway.
The conflict will come from increment or checkAndxxx which acquires write lock
on the row. But we don't batch such ops. It is one at a time.
It is either a or b or c. Sorting the original mini batch from the other
thread from (a, c, b) to (a, b, c) does not seem to help in this case either.
It is possible the user can explicitly request mutateRowsWithLocks for multiple
rows in a coprocessor. But it is a rare case.
I wonder if I misunderstand anything ...
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16018713#comment-16018713
]
Allan Yang commented on HBASE-17924:
{quote}
Thanks for pointing me here from HBASE-18074 Allan Yang. Did you find any perf
benefit sorting?
{quote}
No, we haven't run some performance data for the patch yet, I will provide some
if I have time to do so. But the patch should help with situation which
parallel threads are executing multi with disordered keys(some kyes are the
same).
{quote}
The sort costs but knowing that mutations coming in are sorted, perhaps in
doMiniBatch I could have a single lock cover all mutations on the same row that
follow (but this would involve a bunch of row compares).
{quote}
Now, row lock is a ReadWrite lock. It is a reentrantlock. I don't know which is
more costing, lock the same row or compare the rows, avoiding lock the same row
twice. If locking the locked reentrantlock is more costing than compare the
rows in bytes, then I think your idea is doable。
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16017419#comment-16017419
]
stack commented on HBASE-17924:
---
Thanks for pointing me here from HBASE-18074 [~allan163]. Did you find any perf
benefit sorting? The sort costs but knowing that mutations coming in are
sorted, perhaps in doMiniBatch I could have a single lock cover all mutations
on the same row that follow (but this would involve a bunch of row compares).
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16002019#comment-16002019
]
Hudson commented on HBASE-17924:
SUCCESS: Integrated in Jenkins build HBase-Trunk_matrix #2976 (See
[https://builds.apache.org/job/HBase-Trunk_matrix/2976/])
HBASE-17924 Consider sorting the row order when processing multi() ops
(apurtell: rev 959deb0e5c7c43c2cda6fe4b5c4d46ed80d3d8e6)
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
* (edit) hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALSplitter.java
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16001954#comment-16001954
]
Hudson commented on HBASE-17924:
FAILURE: Integrated in Jenkins build HBase-1.4 #726 (See
[https://builds.apache.org/job/HBase-1.4/726/])
HBASE-17924 Consider sorting the row order when processing multi() ops
(apurtell: rev 0f6a2c41133b2d800389be81f3aefe4589ec1625)
* (edit)
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
* (edit) hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALSplitter.java
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0, 1.4.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15998971#comment-15998971
]
Andrew Purtell commented on HBASE-17924:
Thanks [~allan163]. v5 looks better. Let me check a few things then will commit.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15992324#comment-15992324
]
Hadoop QA commented on HBASE-17924:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 23s
{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s {color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s
{color} | {color:red} 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. {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 6m
10s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 20s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
25s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
25s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 4m 7s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 53s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
28s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 16s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 16s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
26s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
24s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
54m 59s {color} | {color:green} Patch does not cause any errors with Hadoop
2.6.1 2.6.2 2.6.3 2.6.4 2.6.5 2.7.1 2.7.2 2.7.3 or 3.0.0-alpha2. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 4m
16s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 49s
{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 110m 7s {color}
| {color:red} hbase-server in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
39s {color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 190m 44s {color}
| {color:black} {color} |
\\
\\
|| Reason || Tests ||
| Timed out junit tests |
org.apache.hadoop.hbase.snapshot.TestSecureExportSnapshot |
| | org.apache.hadoop.hbase.snapshot.TestExportSnapshot |
| | org.apache.hadoop.hbase.client.TestRestoreSnapshotFromClient |
| | org.apache.hadoop.hbase.client.TestSnapshotFromClient |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.03.0-ce Server=17.03.0-ce Image:yetus/hbase:8d52d23 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12865857/HBASE-17924.v5.patch |
| JIRA Issue | HBASE-17924 |
| Optional Tests | asflicense javac javadoc unit findbugs hadoopcheck
hbaseanti checkstyle compile |
| uname | Linux d3a1929f0ba0 4.8.3-std-1 #1 SMP Fri Oct 21 11:15:43 UTC 2016
x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
|
| git revision | master / 13b6fdf |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
| unit |
https://builds.apache.org/job/PreCommit-HBASE-Build/6655/artifact/patchprocess/patch-unit-hbase-server.txt
|
| unit test logs |
https://builds.apache.org/job/PreCommit-HBASE-Build/6655/artifact/patchprocess/patch-unit-hbase-server.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-HBASE-Build/6655/testReport/ |
| modules | C: hbase-server U: hbase-server |
| Console output |
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15991944#comment-15991944
]
Allan Yang commented on HBASE-17924:
Sorry, I messed up some code, some line is missing, v5 patch is attached, it
should be the right version. Thanks, [~apurtell] for reviewing!
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch,
> HBASE-17924.v5.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15991767#comment-15991767
]
Andrew Purtell commented on HBASE-17924:
[~allan163] Apologies for the delay. I'm looking at the V4 patch. I don't see
where you add anything to mutationActionMap, which is initialized as empty. Is
this some kind of incremental diff against earlier patches? Please attach a
full diff from the base source. That can also explain a failed QA run.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15980822#comment-15980822
]
Allan Yang commented on HBASE-17924:
ping [~apurtell], please review the latest patch. Those UT fails seems not
related. Maybe something wrong with Hudson env, I don't see any pre-commit job
success these days.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch,
> HBASE-17924.v2.patch, HBASE-17924.v3.patch, HBASE-17924.v4.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15978052#comment-15978052
]
Hadoop QA commented on HBASE-17924:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 28s
{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s {color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s
{color} | {color:red} 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. {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 6m
23s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 16s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
27s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
26s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 4m 9s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 51s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
29s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 34s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 34s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
43s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
33s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
58m 12s {color} | {color:green} Patch does not cause any errors with Hadoop
2.6.1 2.6.2 2.6.3 2.6.4 2.6.5 2.7.1 2.7.2 2.7.3 or 3.0.0-alpha2. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 4m
12s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 51s
{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 64m 53s {color}
| {color:red} hbase-server in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
16s {color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 149m 15s {color}
| {color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests | hadoop.hbase.backup.TestBackupHFileCleaner |
| | hadoop.hbase.regionserver.TestHRegionFileSystem |
| | hadoop.hbase.master.locking.TestLockProcedure |
| | hadoop.hbase.master.locking.TestLockManager |
| | hadoop.hbase.procedure.TestProcedureManager |
| | hadoop.hbase.master.balancer.TestRegionLocationFinder |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.03.0-ce Server=17.03.0-ce Image:yetus/hbase:8d52d23 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12864410/HBASE-17924.v4.patch |
| JIRA Issue | HBASE-17924 |
| Optional Tests | asflicense javac javadoc unit findbugs hadoopcheck
hbaseanti checkstyle compile |
| uname | Linux 0de54256086c 4.8.3-std-1 #1 SMP Fri Oct 21 11:15:43 UTC 2016
x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
|
| git revision | master / 49cba2c |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
| unit |
https://builds.apache.org/job/PreCommit-HBASE-Build/6523/artifact/patchprocess/patch-unit-hbase-server.txt
|
| unit test logs |
https://builds.apache.org/job/PreCommit-HBASE-Build/6523/artifact/patchprocess/patch-unit-hbase-server.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-HBASE-Build/6523/testReport/ |
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15976766#comment-15976766
]
Hadoop QA commented on HBASE-17924:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 27s
{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s {color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s
{color} | {color:red} 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. {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 6m
6s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 16s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
26s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
25s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 4m 1s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 50s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
25s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 14s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 14s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
29s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
30s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
57m 32s {color} | {color:green} Patch does not cause any errors with Hadoop
2.6.1 2.6.2 2.6.3 2.6.4 2.6.5 2.7.1 2.7.2 2.7.3 or 3.0.0-alpha2. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 5m 2s
{color} | {color:red} hbase-server generated 2 new + 0 unchanged - 0 fixed = 2
total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m 5s
{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 173m 26s
{color} | {color:red} hbase-server in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
34s {color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 257m 24s {color}
| {color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hbase-server |
| | Dead store to mutationActionMap in
org.apache.hadoop.hbase.regionserver.RSRpcServices.doBatchOp(ClientProtos$RegionActionResult$Builder,
Region, OperationQuota, List, CellScanner) At
RSRpcServices.java:org.apache.hadoop.hbase.regionserver.RSRpcServices.doBatchOp(ClientProtos$RegionActionResult$Builder,
Region, OperationQuota, List, CellScanner) At RSRpcServices.java:[line 886] |
| | org.apache.hadoop.hbase.wal.WALSplitter$MutationReplay defines equals and
uses Object.hashCode() At WALSplitter.java:Object.hashCode() At
WALSplitter.java:[lines 2305-2308] |
| Failed junit tests | hadoop.hbase.snapshot.TestExportSnapshot |
| | hadoop.hbase.snapshot.TestMobExportSnapshot |
| | hadoop.hbase.snapshot.TestMobSecureExportSnapshot |
| Timed out junit tests |
org.apache.hadoop.hbase.master.cleaner.TestSnapshotFromMaster |
| | org.apache.hadoop.hbase.master.TestWarmupRegion |
| | org.apache.hadoop.hbase.filter.TestFuzzyRowFilterEndToEnd |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.03.0-ce Server=17.03.0-ce Image:yetus/hbase:8d52d23 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12864224/HBASE-17924.v3.patch |
| JIRA Issue | HBASE-17924 |
| Optional Tests | asflicense javac javadoc unit findbugs hadoopcheck
hbaseanti
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15976104#comment-15976104
]
Hadoop QA commented on HBASE-17924:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 36s
{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s {color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s
{color} | {color:red} 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. {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 7m
39s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 42s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
37s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
39s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 5m 5s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m 11s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
56s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 44s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 44s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
43s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
34s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
63m 50s {color} | {color:green} Patch does not cause any errors with Hadoop
2.6.1 2.6.2 2.6.3 2.6.4 2.6.5 2.7.1 2.7.2 2.7.3 or 3.0.0-alpha2. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 4m 46s
{color} | {color:red} hbase-server generated 1 new + 0 unchanged - 0 fixed = 1
total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m 2s
{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 61m 4s {color}
| {color:red} hbase-server in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
16s {color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 156m 9s {color}
| {color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hbase-server |
| | org.apache.hadoop.hbase.wal.WALSplitter$MutationReplay defines equals and
uses Object.hashCode() At WALSplitter.java:Object.hashCode() At
WALSplitter.java:[lines 2306-2309] |
| Failed junit tests | hadoop.hbase.master.locking.TestLockProcedure |
| | hadoop.hbase.master.balancer.TestRegionLocationFinder |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.03.0-ce Server=17.03.0-ce Image:yetus/hbase:8d52d23 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12864186/HBASE-17924.v2.patch |
| JIRA Issue | HBASE-17924 |
| Optional Tests | asflicense javac javadoc unit findbugs hadoopcheck
hbaseanti checkstyle compile |
| uname | Linux d63e8af8aad2 4.8.3-std-1 #1 SMP Fri Oct 21 11:15:43 UTC 2016
x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build@2/component/dev-support/hbase-personality.sh
|
| git revision | master / 3acd8e4 |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
| findbugs |
https://builds.apache.org/job/PreCommit-HBASE-Build/6508/artifact/patchprocess/new-findbugs-hbase-server.html
|
| unit |
https://builds.apache.org/job/PreCommit-HBASE-Build/6508/artifact/patchprocess/patch-unit-hbase-server.txt
|
| unit test logs |
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15973863#comment-15973863
]
Allan Yang commented on HBASE-17924:
{quote}
Test failure looks related to patch.
org.apache.hadoop.hbase.wal.WALSplitter$MutationReplay defines
compareTo(WALSplitter$MutationReplay) and uses Object.equals() At
WALSplitter.java:Object.equals() At WALSplitter.java:[line 2301]
{quote}
Yes, There is a bug in the patch, I'm working on it
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15973693#comment-15973693
]
Enis Soztutar commented on HBASE-17924:
---
Good point, we do this sorting in MultiRowMutationEndpoint:
{code}
// set of rows to lock, sorted to avoid deadlocks
SortedSet rowsToLock = new TreeSet<>(Bytes.BYTES_COMPARATOR);
List mutateRequestList = request.getMutationRequestList();
List mutations = new ArrayList<>(mutateRequestList.size());
for (MutationProto m : mutateRequestList) {
mutations.add(ProtobufUtil.toMutation(m));
}
{code}
Makes sense to do it in the regular batch path as well.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15973270#comment-15973270
]
Andrew Purtell commented on HBASE-17924:
Test failure looks related to patch.
org.apache.hadoop.hbase.wal.WALSplitter$MutationReplay defines
compareTo(WALSplitter$MutationReplay) and uses Object.equals() At
WALSplitter.java:Object.equals() At WALSplitter.java:[line 2301]
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Affects Versions: 2.0.0, 1.1.8
>Reporter: Andrew Purtell
>Assignee: Allan Yang
> Fix For: 2.0.0
>
> Attachments: HBASE-17924.patch, HBASE-17924.v0.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15972090#comment-15972090
]
Hadoop QA commented on HBASE-17924:
---
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 29s
{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s {color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s
{color} | {color:red} 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. {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 6m
2s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 18s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
25s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
26s {color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 4m 1s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 54s
{color} | {color:green} master passed {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
26s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m 20s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m 20s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 1m
24s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
26s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
54m 47s {color} | {color:green} Patch does not cause any errors with Hadoop
2.6.1 2.6.2 2.6.3 2.6.4 2.6.5 2.7.1 2.7.2 2.7.3 or 3.0.0-alpha2. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 4m 24s
{color} | {color:red} hbase-server generated 1 new + 0 unchanged - 0 fixed = 1
total (was 0) {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 49s
{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 111m 25s
{color} | {color:red} hbase-server in the patch failed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
32s {color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 191m 38s {color}
| {color:black} {color} |
\\
\\
|| Reason || Tests ||
| FindBugs | module:hbase-server |
| | org.apache.hadoop.hbase.wal.WALSplitter$MutationReplay defines
compareTo(WALSplitter$MutationReplay) and uses Object.equals() At
WALSplitter.java:Object.equals() At WALSplitter.java:[line 2301] |
| Failed junit tests | hadoop.hbase.regionserver.TestHRegion |
| | hadoop.hbase.regionserver.TestHRegionWithInMemoryFlush |
| | hadoop.hbase.regionserver.TestPerColumnFamilyFlush |
| Timed out junit tests |
org.apache.hadoop.hbase.security.visibility.TestVisibilityLabelsWithDefaultVisLabelService
|
| |
org.apache.hadoop.hbase.security.access.TestCoprocessorWhitelistMasterObserver |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=17.03.0-ce Server=17.03.0-ce Image:yetus/hbase:8d52d23 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12863714/HBASE-17924.patch |
| JIRA Issue | HBASE-17924 |
| Optional Tests | asflicense javac javadoc unit findbugs hadoopcheck
hbaseanti checkstyle compile |
| uname | Linux 2c4f3a1366c8 4.8.3-std-1 #1 SMP Fri Oct 21 11:15:43 UTC 2016
x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
|
| git revision | master / ecdfb82 |
| Default Java | 1.8.0_121 |
| findbugs | v3.0.0 |
|
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15971904#comment-15971904
]
Allan Yang commented on HBASE-17924:
OK, I will assign this issue to me and trigger a test
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Reporter: Andrew Purtell
> Attachments: HBASE-17924.v0.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15971387#comment-15971387
]
Andrew Purtell commented on HBASE-17924:
+1
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Reporter: Andrew Purtell
> Attachments: HBASE-17924.v0.patch
>
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15970575#comment-15970575
]
Allan Yang commented on HBASE-17924:
It won't be a deadlock in batch ops even if not sorted. Since when taking the
row lock, the thread will not wait for the lock, instead, it will fail fast
then collecting the rows which are not successfully written, and try them
again. Please refer to step1 in {{doMiniBatchMutate}}.
But, I also thinking that ordering rows before doing any operations is better.
Though now it can retry when failed to acquire locks, sorting may decrease the
possibility of those fails a little bit and increase the performance.
If you don't mind, I can provide a patch, we already have done the sorting in
our branch of code.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Reporter: Andrew Purtell
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15969806#comment-15969806
]
Jerry He commented on HBASE-17924:
--
Yeah, it is a common cause for deadlock.
But I am surprised that we have not seen such reported deadlocks much. It is
in the common code path.
It does need to be exclusive locks, right?
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Reporter: Andrew Purtell
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15969714#comment-15969714
]
James Taylor commented on HBASE-17924:
--
The reason you'd want to take the locks in a known, consistent manner is to
prevent deadlocks. Consider a case with two clients each modifying the same two
rows in a single batch. If client #1 submits a batch with rows {A, B} and
client #2 submits a batch with rows {B, A} you have the potential to deadlock
without ordering the locks in some consistent, predictable way wrt each other,
for example by row key. It's the classic "Dining Philosophers" problem
(https://en.wikipedia.org/wiki/Dining_philosophers_problem).
This could be completely transparent to the client as it would only impact the
order the locks are taken and released in - the rows could still be processed
in the order they were sent over from the client.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Reporter: Andrew Purtell
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17924) Consider sorting the row order when processing multi() ops before taking rowlocks
[
https://issues.apache.org/jira/browse/HBASE-17924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15969474#comment-15969474
]
Jerry He commented on HBASE-17924:
--
Nice and clear illustration of the flow!
For the first case, there would not be any problem for sure. It is atomic,
either non or all.
> Consider sorting the row order when processing multi() ops before taking
> rowlocks
> -
>
> Key: HBASE-17924
> URL: https://issues.apache.org/jira/browse/HBASE-17924
> Project: HBase
> Issue Type: Improvement
>Reporter: Andrew Purtell
>
> When processing a batch mutation, we take row locks in whatever order the
> mutations were added to the multi op by the client.
>
> {noformat}
> RSRpcServices#multi -> RSRpcServices#mutateRows -> HRegion#mutateRow ->
> HRegion#mutateRowsWithLocks -> HRegion#processRowsWithLocks
> {noformat}
> Or
> {noformat}
> RSRpcServices#multi -> RSRpcServices#doNonAtomicRegionMutation ->
> HRegion#get
> | HRegion#append
> | HRegion#increment
> | HRegionServer#doBatchOp -> HRegion#batchMutate ->
> HRegion#doMiniBatchMutation
> {noformat}
>
> multi() is fed by client APIs that accept a RowMutations object containing
> actions for multiple rows. The container for ops inside RowMutations is an
> ArrayList, which doesn't change the ordering of objects added to it. The
> protobuf implementation of the messages for multi ops do not reorder the list
> of actions. When processing multi ops we iterate over the actions in the
> order rehydrated from protobuf.
> We should discuss sorting the order of ops by row key when processing multi()
> ops before taking row locks. Does this make lock ordering more predictable
> for server side operations? Yes, but potentially surprising for the client,
> right? Is there any legitimate reason we should take locks out of row key
> sorted order because the client has structured the request as such?
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
