[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/incubator-tephra/pull/35


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100940812
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java
 ---
@@ -97,27 +101,33 @@ private void startFlushThread() {
 flushThread = new Thread("tephra-prune-upper-bound-writer") {
   @Override
   public void run() {
-while ((!isInterrupted()) && isRunning()) {
+while ((!isInterrupted()) && (!stopped)) {
   long now = System.currentTimeMillis();
   if (now > (lastChecked + pruneFlushInterval)) {
 // should flush data
 try {
-  // Record prune upper bound
-  while (!pruneEntries.isEmpty()) {
-Map.Entry firstEntry = 
pruneEntries.firstEntry();
-
dataJanitorState.savePruneUpperBoundForRegion(firstEntry.getKey(), 
firstEntry.getValue());
-// We can now remove the entry only if the key and value 
match with what we wrote since it is
-// possible that a new pruneUpperBound for the same key 
has been added
-pruneEntries.remove(firstEntry.getKey(), 
firstEntry.getValue());
-  }
-  // Record empty regions
-  while (!emptyRegions.isEmpty()) {
-Map.Entry firstEntry = 
emptyRegions.firstEntry();
-
dataJanitorState.saveEmptyRegionForTime(firstEntry.getValue(), 
firstEntry.getKey());
-// We can now remove the entry only if the key and value 
match with what we wrote since it is
-// possible that a new value for the same key has been 
added
-emptyRegions.remove(firstEntry.getKey(), 
firstEntry.getValue());
-  }
+  User.runAsLoginUser(new PrivilegedExceptionAction() {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100940705
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java
 ---
@@ -454,28 +442,38 @@ protected Filter getTransactionFilter(Transaction tx, 
ScanType type, Filter filt
 return TransactionFilters.getVisibilityFilter(tx, ttlByFamily, 
allowEmptyValues, type, filter);
   }
 
-  private void 
initPruneState(ObserverContext c) {
-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) {
--- End diff --

Good to add javadoc for the protected method


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100931804
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java
 ---
@@ -454,28 +441,39 @@ protected Filter getTransactionFilter(Transaction tx, 
ScanType type, Filter filt
 return TransactionFilters.getVisibilityFilter(tx, ttlByFamily, 
allowEmptyValues, type, filter);
   }
 
-  private void 
initPruneState(ObserverContext c) {
-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) {
+Configuration conf = getConfiguration(env);
 if (conf != null) {
   pruneEnable = 
conf.getBoolean(TxConstants.TransactionPruning.PRUNE_ENABLE,
 
TxConstants.TransactionPruning.DEFAULT_PRUNE_ENABLE);
+
   if (Boolean.TRUE.equals(pruneEnable)) {
-String pruneTable = 
conf.get(TxConstants.TransactionPruning.PRUNE_STATE_TABLE,
- 
TxConstants.TransactionPruning.DEFAULT_PRUNE_STATE_TABLE);
-long pruneFlushInterval = TimeUnit.SECONDS.toMillis(
-  conf.getLong(TxConstants.TransactionPruning.PRUNE_FLUSH_INTERVAL,
-   
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.
+TableName pruneTable = 
TableName.valueOf(conf.get(TxConstants.TransactionPruning.PRUNE_STATE_TABLE,
+  
TxConstants.TransactionPruning.DEFAULT_PRUNE_STATE_TABLE));
+long pruneFlushInterval = TimeUnit.SECONDS.toMillis(conf.getLong(
+  TxConstants.TransactionPruning.PRUNE_FLUSH_INTERVAL,
+  TxConstants.TransactionPruning.DEFAULT_PRUNE_FLUSH_INTERVAL));
+
+compactionState = new CompactionState(env, pruneTable, 
pruneFlushInterval);
 if (LOG.isDebugEnabled()) {
-  LOG.debug("Automatic invalid list pruning is enabled. Compaction 
state will be recorded in table "
-  + pruneTable);
+  LOG.debug(String.format("Automatic invalid list pruning is 
enabled for table %s. Compaction state " +
+"will be recorded in table %s",
+  
env.getRegionInfo().getTable().getNameWithNamespaceInclAsString(),
+  
pruneTable.getNameWithNamespaceInclAsString()));
 }
   }
 }
   }
 
+  protected void resetPruneState() {
+pruneEnable = false;
+if (compactionState != null) {
+  compactionState.stop();
--- End diff --

It would be good to reset `compactionState` to `null` here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100932301
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java
 ---
@@ -356,8 +356,8 @@ public InternalScanner 
preCompactScannerOpen(ObserverContext e, 
Store store, StoreFile resultFile,
   CompactionRequest request) throws IOException {
-// Persist the compaction state after a succesful compaction
-if (compactionState != null) {
+// Persist the compaction state after a successful compaction
+if (Boolean.TRUE.equals(pruneEnable)) {
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100931439
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java
 ---
@@ -340,12 +345,7 @@ public InternalScanner 
preCompactScannerOpen(ObserverContext

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100931390
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java
 ---
@@ -454,28 +441,39 @@ protected Filter getTransactionFilter(Transaction tx, 
ScanType type, Filter filt
 return TransactionFilters.getVisibilityFilter(tx, ttlByFamily, 
allowEmptyValues, type, filter);
   }
 
-  private void 
initPruneState(ObserverContext c) {
-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) {
+Configuration conf = getConfiguration(env);
 if (conf != null) {
   pruneEnable = 
conf.getBoolean(TxConstants.TransactionPruning.PRUNE_ENABLE,
 
TxConstants.TransactionPruning.DEFAULT_PRUNE_ENABLE);
+
   if (Boolean.TRUE.equals(pruneEnable)) {
-String pruneTable = 
conf.get(TxConstants.TransactionPruning.PRUNE_STATE_TABLE,
- 
TxConstants.TransactionPruning.DEFAULT_PRUNE_STATE_TABLE);
-long pruneFlushInterval = TimeUnit.SECONDS.toMillis(
-  conf.getLong(TxConstants.TransactionPruning.PRUNE_FLUSH_INTERVAL,
-   
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.
--- End diff --

This comment can be removed now


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[gokulavasan]

Github user gokulavasan commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100881247
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java
 ---
@@ -81,14 +84,23 @@ private void startFlushThread() {
 flushThread = new Thread("tephra-prune-upper-bound-writer") {
   @Override
   public void run() {
-while ((!isInterrupted()) && isRunning()) {
+Service.State serviceState = state();
+while ((!isInterrupted()) &&
+  (serviceState.equals(Service.State.NEW) || 
serviceState.equals(Service.State.STARTING) ||
--- End diff --

Yes might be simpler


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100709175
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java
 ---
@@ -81,14 +84,23 @@ private void startFlushThread() {
 flushThread = new Thread("tephra-prune-upper-bound-writer") {
   @Override
   public void run() {
-while ((!isInterrupted()) && isRunning()) {
+Service.State serviceState = state();
+while ((!isInterrupted()) &&
+  (serviceState.equals(Service.State.NEW) || 
serviceState.equals(Service.State.STARTING) ||
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100709097
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java
 ---
@@ -317,31 +317,31 @@ public InternalScanner 
preCompactScannerOpen(ObserverContext

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[poornachandra]

Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/35#discussion_r100709949
  
--- Diff: 
tephra-hbase-compat-1.1-base/src/main/java/org/apache/tephra/hbase/txprune/PruneUpperBoundWriter.java
 ---
@@ -81,14 +84,23 @@ private void startFlushThread() {
 flushThread = new Thread("tephra-prune-upper-bound-writer") {
   @Override
   public void run() {
-while ((!isInterrupted()) && isRunning()) {
+Service.State serviceState = state();
+while ((!isInterrupted()) &&
+  (serviceState.equals(Service.State.NEW) || 
serviceState.equals(Service.State.STARTING) ||
+serviceState.equals(Service.State.RUNNING))) {
   long now = System.currentTimeMillis();
   if (now > (lastChecked + pruneFlushInterval)) {
 // should flush data
 try {
   while (pruneEntries.firstEntry() != null) {
-Map.Entry firstEntry = 
pruneEntries.firstEntry();
-
dataJanitorState.savePruneUpperBoundForRegion(firstEntry.getKey(), 
firstEntry.getValue());
+final Map.Entry firstEntry = 
pruneEntries.firstEntry();
+User.runAsLoginUser(new PrivilegedExceptionAction() {
--- End diff --

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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #35: (TEPHRA-219) Execute cross region calls i...

[gokulavasan]

GitHub user gokulavasan opened a pull request:

https://github.com/apache/incubator-tephra/pull/35

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

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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/gokulavasan/incubator-tephra 
feature/dynamic-loading-props

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-tephra/pull/35.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #35


commit 5fd4c6f54f9c17500788f1ae7bea38be073729fc
Author: Gokul Gunasekaran 
Date:   2017-02-12T03:42:02Z

Check all valid states

commit 4b7a837ae8584c18437984bf13579d31628f9487
Author: Gokul Gunasekaran 
Date:   2017-02-12T07:13:53Z

Load properties dynamically

commit 2c8b8952025adaa7cef96c90a9d60ffcce233da4
Author: Gokul Gunasekaran 
Date:   2017-02-12T07:41:58Z

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

commit 9a120fe2db1a6e99f2d81dda00f99346a9cc4457
Author: Gokul Gunasekaran 
Date:   2017-02-12T07:54:45Z

We can;t guarantee to reload pruneTable and pruneFlushInterval since we use 
single thread across all regions and thus the thread might be already running




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---