[
https://issues.apache.org/jira/browse/HBASE-11126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14009866#comment-14009866
]
Andrew Purtell commented on HBASE-11126:
----------------------------------------
Patch is looking good.
{code}
@@ -587,6 +587,25 @@ public interface RegionObserver extends Coprocessor {
void preDelete(final ObserverContext<RegionCoprocessorEnvironment> c,
final Delete delete, final WALEdit edit, final Durability durability)
throws IOException;
+/**
+ * Called before the client updates the timestamp for delete Columns with
latest timestamp.
+ * <p>
+ * Call CoprocessorEnvironment#bypass to skip default actions
+ * <p>
+ * Call CoprocessorEnvironment#complete to skip any subsequent chained
+ * coprocessors
+ * @param c the environment provided by the region server
+ * @param mutation - the parent mutation associated with this delete cell
+ * @param cell - The deleteColumn with latest version cell
+ * @param byteNow - timestamp bytes
+ * @param family - family bytes
+ * @param get - the get formed using the current cell's row.
+ * Note that the get does not specify the family and qualifier
+ * @throws IOException
+ */
+ void preGetForDeleteVersion(final
ObserverContext<RegionCoprocessorEnvironment> c,
+ final Mutation mutation, final Cell cell, final byte[] byteNow, final
byte[] family,
+ final Get get) throws IOException;
{code}
preGetForDeleteVersion needs a better name. What are you doing with this new
hook?
In the Javadoc did you mean "Called before the *server* ... "
{code}
@@ -5064,12 +5090,18 @@ public class HRegion implements HeapSize { // ,
Writable{
rowLock = getRowLock(row);
try {
lock(this.updatesLock.readLock());
- // wait for all prior MVCC transactions to finish - while we hold the
row lock
- // (so that we are guaranteed to see the latest state)
- mvcc.completeMemstoreInsert(mvcc.beginMemstoreInsert());
- // now start my own transaction
- w = mvcc.beginMemstoreInsert();
try {
+ // wait for all prior MVCC transactions to finish - while we hold
the row lock
+ // (so that we are guaranteed to see the latest state)
+ mvcc.completeMemstoreInsert(mvcc.beginMemstoreInsert());
+ if (this.coprocessorHost != null) {
+ Result r = this.coprocessorHost.preAppendAfterRowLock(append);
+ if(r!= null) {
+ return r;
+ }
+ }
+ // now start my own transaction
+ w = mvcc.beginMemstoreInsert();
long now = EnvironmentEdgeManager.currentTimeMillis();
// Process each family
for (Map.Entry<byte[], List<Cell>> family :
append.getFamilyCellMap().entrySet()) {
{code}
Why are you moving completeMemstoreInsert and beginMemstoreInsert into the
inner try?
{code}
@@ -5254,12 +5286,18 @@ public class HRegion implements HeapSize { // ,
Writable{
RowLock rowLock = getRowLock(row);
try {
lock(this.updatesLock.readLock());
- // wait for all prior MVCC transactions to finish - while we hold the
row lock
- // (so that we are guaranteed to see the latest state)
- mvcc.completeMemstoreInsert(mvcc.beginMemstoreInsert());
- // now start my own transaction
- w = mvcc.beginMemstoreInsert();
try {
+ // wait for all prior MVCC transactions to finish - while we hold
the row lock
+ // (so that we are guaranteed to see the latest state)
+ mvcc.completeMemstoreInsert(mvcc.beginMemstoreInsert());
+ if (this.coprocessorHost != null) {
+ Result r =
this.coprocessorHost.preIncrementAfterRowLock(increment);
+ if (r != null) {
+ return r;
+ }
+ }
+ // now start my own transaction
+ w = mvcc.beginMemstoreInsert();
long now = EnvironmentEdgeManager.currentTimeMillis();
// Process each family
for (Map.Entry<byte [], List<Cell>> family:
{code}
Ditto.
{code}
diff --git
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
index 2e7b022..e0363b2 100644
---
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
+++
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
@@ -1730,7 +1730,7 @@ public class RSRpcServices implements
HBaseRPCErrorHandler,
ProtobufUtil.toComparator(condition.getComparator());
if (region.getCoprocessorHost() != null) {
processed = region.getCoprocessorHost().preCheckAndPut(
- row, family, qualifier, compareOp, comparator, put);
+ row, family, qualifier, compareOp, comparator, put);
}
if (processed == null) {
boolean result = region.checkAndMutate(row, family,
@@ -1758,7 +1758,7 @@ public class RSRpcServices implements
HBaseRPCErrorHandler,
ProtobufUtil.toComparator(condition.getComparator());
if (region.getCoprocessorHost() != null) {
processed = region.getCoprocessorHost().preCheckAndDelete(
- row, family, qualifier, compareOp, comparator, delete);
+ row, family, qualifier, compareOp, comparator, delete);
}
if (processed == null) {
boolean result = region.checkAndMutate(row, family,
{code}
Whitespace only changes, please remove.
> Add RegionObserver pre hooks that operate under row lock
> --------------------------------------------------------
>
> Key: HBASE-11126
> URL: https://issues.apache.org/jira/browse/HBASE-11126
> Project: HBase
> Issue Type: Improvement
> Affects Versions: 0.99.0, 0.98.3
> Reporter: Andrew Purtell
> Assignee: ramkrishna.s.vasudevan
> Attachments: HBASE-11126.patch, HBASE-11126_1.patch,
> HBASE-11126_new_2.patch, HBASE-11126_new_3.patch
>
>
> The coprocessor hooks were placed outside of row locks. This was meant to
> sidestep performance issues arising from significant work done within hook
> invocations. However as the security code increases in sophistication we are
> now running into concurrency issues trying to use them as a result of that
> early decision. Since the initial introduction of coprocessor upcalls there
> has been some significant refactoring done around them and concurrency
> control in core has become more complex. This is potentially an issue for
> many coprocessor users.
> We should do either:\\
> - Move all existing RegionObserver pre* hooks to execute under row lock.
> - Introduce a new set of RegionObserver pre* hooks that execute under row
> lock, named to indicate such.
> The second option is less likely to lead to surprises.
> All RegionObserver hook Javadoc should be updated with advice to the
> coprocessor implementor not to take their own row locks in the hook. If the
> current thread happens to already have a row lock and they try to take a lock
> on another row, there is a deadlock risk.
> As always a drawback of adding hooks is the potential for performance impact.
> We should benchmark the impact and decide if the second option above is a
> viable choice or if the first option is required.
> Finally, we should introduce a higher level interface for managing the
> registration of 'user' code for execution from the low level hooks. I filed
> HBASE-11125 to discuss this further.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
