(TEPHRA-219) Execute cross region calls in Coprocessor as the login user #35

gokulavasan · 2017-02-12T07:55:05Z

i) Fixes a problem where the pruneThread would exit if the state of the service is not yet set to RUNNING state. This also fixes the flakiness in the PruneUpperBoundSupplierTest which was caused due to the above problem.

ii) Load the txMaxLifeTimeMillis and pruneEnable properties dynamically.

iii) Add a hook around cross region calls in the PruneUpperBoundWriter.

poornachandra

@gokulavasan I have a couple of comments. Also the build is still failing at the prune writer test.

poornachandra · 2017-02-12T23:59:12Z

...-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java

-            LOG.debug("Automatic invalid list pruning is enabled. Compaction state will be recorded in table "
-                        + pruneTable);
-          }
+    Configuration conf = getConfiguration(c.getEnvironment());


We don't need to read the configuration every time since it does not change when part of hbase-site.xml, right? We just need to make sure that when derived classes update the pruneEnable and other fields from a different thread, it gets reflected here.

poornachandra · 2017-02-13T00:02:28Z

...ase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java

-        while ((!isInterrupted()) && isRunning()) {
+        Service.State serviceState = state();
+        while ((!isInterrupted()) &&
+          (serviceState.equals(Service.State.NEW) || serviceState.equals(Service.State.STARTING) ||


hmm - do you think it would simplify the code if we go back to using a stop flag?

Yes might be simpler

poornachandra · 2017-02-13T00:29:46Z

...ase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java

-                Map.Entry<byte[], Long> firstEntry = pruneEntries.firstEntry();
-                dataJanitorState.savePruneUpperBoundForRegion(firstEntry.getKey(), firstEntry.getValue());
+                final Map.Entry<byte[], Long> firstEntry = pruneEntries.firstEntry();
+                User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {


Adding User.runAsLoginUser here brings in unnecessary security code into PruneUpperBoundWriter class. Also if we modify TransactionProcessor later to add other table operations, we may miss to wrap those calls. I think it would be better to do the wrapping in TransactionProcessor class itself. What do you say?

gokulavasan · 2017-02-13T21:20:14Z

@poornachandra The build is failing in a different compat module due to the PruneUpperBoundWriter issue that I have fixed in compat-1.1-base module. Once we port the changes to other modules, it should fix the build.

poornachandra

@gokulavasan Just a couple of minor comments

poornachandra · 2017-02-14T00:07:03Z

...-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java

-                       TxConstants.TransactionPruning.DEFAULT_PRUNE_FLUSH_INTERVAL));
-        compactionState = new CompactionState(c.getEnvironment(), TableName.valueOf(pruneTable), pruneFlushInterval);
+        // pruneTable and pruneFlushInterval cannot be changed by simply loading the Configuration dynamically
+        // since we have only one flush thread across all regions and we might loose.


This comment can be removed now

poornachandra · 2017-02-14T00:07:26Z

...-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java

    if (Boolean.TRUE.equals(pruneEnable)) {
-      // Record tx state before the compaction


This comment is still relevant, no?

poornachandra · 2017-02-14T00:09:47Z

...-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java

+  protected void resetPruneState() {
+    pruneEnable = false;
+    if (compactionState != null) {
+      compactionState.stop();


It would be good to reset compactionState to null here.

poornachandra · 2017-02-14T00:13:29Z

...-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java

-    // Persist the compaction state after a succesful compaction
-    if (compactionState != null) {
+    // Persist the compaction state after a successful compaction
+    if (Boolean.TRUE.equals(pruneEnable)) {


It would be better to change this check to compactionState != null. Otherwise there is a race condition during initializePruneState(), where pruneEnable could be true, but compactionState is still not initialized.

poornachandra

@gokulavasan Just one comment on a javadoc. Please fix and port to other compat modules.

poornachandra · 2017-02-14T01:17:29Z

...-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java

-    Configuration conf = getConfiguration(c.getEnvironment());
-    // Configuration won't be null in TransactionProcessor but the derived classes might return
-    // null if it is not available temporarily
+  protected void initializePruneState(RegionCoprocessorEnvironment env) {


Good to add javadoc for the protected method

poornachandra · 2017-02-14T01:18:32Z

...ase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java

-                // possible that a new value for the same key has been added
-                emptyRegions.remove(firstEntry.getKey(), firstEntry.getValue());
-              }
+              User.runAsLoginUser(new PrivilegedExceptionAction<Void>() {


Let's not close TEPHRA-219 until we have figured out how to handle the wrapping cleanly.

poornachandra · 2017-02-14T04:30:26Z

LGTM

Make pruneEnable and txMaxLifetimeMillis volatile so that derived classes can make use of it. Introduced stopped variable in PruneUpperBoundWriter.

poornachandra reviewed Feb 13, 2017

View reviewed changes

gokulavasan force-pushed the feature/dynamic-loading-props branch from 18fa4fc to 9bc97ee Compare February 13, 2017 21:19

gokulavasan force-pushed the feature/dynamic-loading-props branch 2 times, most recently from 7692066 to 909adcb Compare February 13, 2017 21:59

poornachandra suggested changes Feb 14, 2017

View reviewed changes

poornachandra reviewed Feb 14, 2017

View reviewed changes

gokulavasan force-pushed the feature/dynamic-loading-props branch from 4aa5a5e to b69577a Compare February 14, 2017 02:37

TEPHRA-219 execute cross region calls in coprocessor as the login user.

6533a4c

Make pruneEnable and txMaxLifetimeMillis volatile so that derived classes can make use of it. Introduced stopped variable in PruneUpperBoundWriter.

gokulavasan force-pushed the feature/dynamic-loading-props branch from 2a142f4 to 6533a4c Compare February 14, 2017 04:53

asfgit closed this in 203825e Feb 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(TEPHRA-219) Execute cross region calls in Coprocessor as the login user #35

(TEPHRA-219) Execute cross region calls in Coprocessor as the login user #35

gokulavasan commented Feb 12, 2017

poornachandra left a comment •

edited

poornachandra Feb 12, 2017

poornachandra Feb 13, 2017

gokulavasan Feb 13, 2017

poornachandra Feb 13, 2017

gokulavasan commented Feb 13, 2017

poornachandra left a comment

poornachandra Feb 14, 2017

poornachandra Feb 14, 2017

poornachandra Feb 14, 2017

poornachandra Feb 14, 2017

poornachandra left a comment

poornachandra Feb 14, 2017

poornachandra Feb 14, 2017

poornachandra commented Feb 14, 2017

		if (Boolean.TRUE.equals(pruneEnable)) {
		// Record tx state before the compaction

(TEPHRA-219) Execute cross region calls in Coprocessor as the login user #35

(TEPHRA-219) Execute cross region calls in Coprocessor as the login user #35

Conversation

gokulavasan commented Feb 12, 2017

poornachandra left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gokulavasan commented Feb 13, 2017

poornachandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poornachandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poornachandra commented Feb 14, 2017

poornachandra left a comment •

edited