[jira] [Resolved] (TEPHRA-194) Transaction client should not retry startShort() if an invalid timeout is given
[ https://issues.apache.org/jira/browse/TEPHRA-194?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Andreas Neumann resolved TEPHRA-194. Resolution: Fixed > Transaction client should not retry startShort() if an invalid timeout is > given > > > Key: TEPHRA-194 > URL: https://issues.apache.org/jira/browse/TEPHRA-194 > Project: Tephra > Issue Type: Bug > Components: client >Affects Versions: 0.9.0-incubating, 0.10.0-incubating >Reporter: Andreas Neumann >Assignee: Andreas Neumann > Fix For: 0.10.0-incubating > > > Currently, if an invalid timeout (negative, or too long) is given, the Tx > manager throws an IllegalArgumentException. The thrift client will catch that > and apply the retry strategy. However, in this case, retry is pointless, and > if the strategy is, for example, exponential backoff, if introduces > unneccessary load and latency. > The service should instead throw a meaningful exception for that, so that the > client knows not to retry. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Resolved] (TEPHRA-201) In-progress transactions may become visible when transactions are checkpointed
[ https://issues.apache.org/jira/browse/TEPHRA-201?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Andreas Neumann resolved TEPHRA-201. Resolution: Fixed > In-progress transactions may become visible when transactions are checkpointed > -- > > Key: TEPHRA-201 > URL: https://issues.apache.org/jira/browse/TEPHRA-201 > Project: Tephra > Issue Type: Bug > Components: core >Affects Versions: 0.8.0-incubating, 0.9.0-incubating >Reporter: Poorna Chandra >Assignee: Poorna Chandra >Priority: Blocker > Fix For: 0.10.0-incubating > > > When a transaction is created, the current in-progress list is attached to > the transaction object. This list is used to filter out in-progress > transactions during read. > The transaction object expects this list to be sorted numerically since it > does a binary search for filtering out the in-progress transactions. > When checkpointing feature was added in TEPHRA-96, checkpoints were also > added as part of the in-progress list since data writes from checkpoints also > have to be filtered out during reads. However adding checkpoints broke the > sort order. This leads to binary search not working as expected to filter out > in-progress transactions. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (TEPHRA-201) In-progress transactions may become visible when transactions are checkpointed
[ https://issues.apache.org/jira/browse/TEPHRA-201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15733350#comment-15733350 ] Andreas Neumann commented on TEPHRA-201: PR: https://github.com/apache/incubator-tephra/pull/23 > In-progress transactions may become visible when transactions are checkpointed > -- > > Key: TEPHRA-201 > URL: https://issues.apache.org/jira/browse/TEPHRA-201 > Project: Tephra > Issue Type: Bug > Components: core >Affects Versions: 0.8.0-incubating, 0.9.0-incubating >Reporter: Poorna Chandra >Assignee: Poorna Chandra >Priority: Blocker > Fix For: 0.10.0-incubating > > > When a transaction is created, the current in-progress list is attached to > the transaction object. This list is used to filter out in-progress > transactions during read. > The transaction object expects this list to be sorted numerically since it > does a binary search for filtering out the in-progress transactions. > When checkpointing feature was added in TEPHRA-96, checkpoints were also > added as part of the in-progress list since data writes from checkpoints also > have to be filtered out during reads. However adding checkpoints broke the > sort order. This leads to binary search not working as expected to filter out > in-progress transactions. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[GitHub] incubator-tephra issue #23: [Tephra-201] Store checkpoints in in-progress li...
Github user chtyim commented on the issue: https://github.com/apache/incubator-tephra/pull/23 Just one last comment, the rest LGTM --- 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 #23: [Tephra-201] Store checkpoints in in-prog...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/23#discussion_r91567173
--- Diff:
tephra-core/src/main/java/org/apache/tephra/TransactionManager.java ---
@@ -1011,10 +1038,9 @@ private boolean doInvalidate(long writePointer) {
} else {
// invalidate any checkpoint write pointers
LongArrayList childWritePointers =
previous.getCheckpointWritePointers();
-if (childWritePointers != null) {
- for (int i = 0; i < childWritePointers.size(); i++) {
-invalid.add(childWritePointers.get(i));
- }
+for (long childWritePointer : childWritePointers) {
+ invalid.add(childWritePointer);
+ inProgress.remove(childWritePointer);
--- End diff --
Missed this one in last review, we can use the
`keySet().removeAll(childWritePointers)` as well (and check if
`!childWritePointers.isEmpty()` first). The same for invalid, it can be
`invalid.addAll(childWritePointers)`
---
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 #23: [Tephra-201] Store checkpoints in in-prog...
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/23#discussion_r91471138
--- Diff:
tephra-core/src/main/java/org/apache/tephra/TransactionManager.java ---
@@ -340,48 +340,51 @@ public long getSleepMillis() {
private void cleanupTimedOutTransactions() {
List invalidEdits = null;
-this.logReadLock.lock();
-try {
- synchronized (this) {
-if (!isRunning()) {
- return;
-}
+synchronized (this) {
+ if (!isRunning()) {
+return;
+ }
-long currentTime = System.currentTimeMillis();
-List timedOut = Lists.newArrayList();
-for (Map.Entry tx : inProgress.entrySet()) {
- long expiration = tx.getValue().getExpiration();
- if (expiration >= 0L && currentTime > expiration) {
-// timed out, remember tx id (can't remove while iterating
over entries)
-timedOut.add(tx.getKey());
-LOG.info("Tx invalid list: added tx {} because of timeout",
tx.getKey());
- } else if (expiration < 0) {
-LOG.warn("Transaction {} has negative expiration time {}.
Likely cause is the transaction was not " +
- "migrated correctly, this transaction will be
expired immediately",
- tx.getKey(), expiration);
-timedOut.add(tx.getKey());
- }
+ long currentTime = System.currentTimeMillis();
+ Map timedOut = Maps.newHashMap();
+ for (Map.Entry tx : inProgress.entrySet()) {
+long expiration = tx.getValue().getExpiration();
+if (expiration >= 0L && currentTime > expiration) {
+ // timed out, remember tx id (can't remove while iterating over
entries)
+ timedOut.put(tx.getKey(), tx.getValue().getType());
+ LOG.info("Tx invalid list: added tx {} because of timeout",
tx.getKey());
+} else if (expiration < 0) {
+ LOG.warn("Transaction {} has negative expiration time {}. Likely
cause is the transaction was not " +
+ "migrated correctly, this transaction will be expired
immediately",
+ tx.getKey(), expiration);
+ timedOut.put(tx.getKey(), InProgressType.LONG);
}
-if (!timedOut.isEmpty()) {
+ }
+ if (!timedOut.isEmpty()) {
+logWriteLock.lock();
--- End diff --
agreed. ok. will revert
---
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 #23: [Tephra-201] Store checkpoints in in-prog...
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/23#discussion_r91468098
--- Diff:
tephra-core/src/main/java/org/apache/tephra/TransactionManager.java ---
@@ -348,27 +348,30 @@ private void cleanupTimedOutTransactions() {
}
long currentTime = System.currentTimeMillis();
-List timedOut = Lists.newArrayList();
+Map timedOut = Maps.newHashMap();
--- End diff --
you are right. I can change it here, but it is really not related to this
Jira... will do anyway
---
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 #23: [Tephra-201] Store checkpoints in in-prog...
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/23#discussion_r91467770
--- Diff:
tephra-core/src/main/java/org/apache/tephra/TransactionManager.java ---
@@ -1319,20 +1344,60 @@ public void shutdown() {
}
/**
+ * Type of in-progress transaction.
+ */
+ public enum InProgressType {
+
+/**
+ * Short transactions detect conflicts during commit.
+ */
+SHORT(TransactionType.SHORT),
+
+/**
+ * Long running transactions do not detect conflicts during commit.
+ */
+LONG(TransactionType.LONG),
+
+/**
+ * Check-pointed transactions are recorded as in-progress.
+ */
+CHECKPOINT(null);
+
+private final TransactionType transactionType;
+
+InProgressType(TransactionType transactionType) {
+ this.transactionType = transactionType;
+}
+
+public static InProgressType of(TransactionType type) {
+ switch (type) {
+case SHORT: return SHORT;
+case LONG: return LONG;
+default: throw new IllegalArgumentException("Unknown
TransactionType " + type);
+ }
+}
+
+@Nullable
+public TransactionType getTransactionType() {
+ return transactionType;
+}
+ }
+
+ /**
* Represents some of the info on in-progress tx
*/
public static final class InProgressTx {
/** the oldest in progress tx at the time of this tx start */
private final long visibilityUpperBound;
private final long expiration;
-private final TransactionType type;
+private final InProgressType type;
private LongArrayList checkpointWritePointers = new LongArrayList();
--- End diff --
not introduced by this PR, but fixed 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 #23: [Tephra-201] Store checkpoints in in-prog...
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/23#discussion_r91467537
--- Diff:
tephra-core/src/main/java/org/apache/tephra/TransactionManager.java ---
@@ -958,25 +983,31 @@ private void doAbort(long writePointer, long[]
checkpointWritePointers, Transact
// makes tx visible (assumes that all operations were rolled back)
// remove from in-progress set, so that it does not get excluded in
the future
InProgressTx removed = inProgress.remove(writePointer);
+boolean isInvalid = false;
if (removed == null) {
// tx was not in progress! perhaps it timed out and is invalid? try
to remove it there.
if (invalid.rem(writePointer)) {
+isInvalid = true;
// remove any invalidated checkpoint pointers
// this will only be present if the parent write pointer was also
invalidated
if (checkpointWritePointers != null) {
- for (int i = 0; i < checkpointWritePointers.length; i++) {
-invalid.rem(checkpointWritePointers[i]);
+ for (long checkpointWritePointer : checkpointWritePointers) {
+invalid.rem(checkpointWritePointer);
}
}
invalidArray = invalid.toLongArray();
LOG.info("Tx invalid list: removed aborted tx {}", writePointer);
-// removed a tx from excludes: must move read pointer
-moveReadPointerIfNeeded(writePointer);
}
-} else {
- // removed a tx from excludes: must move read pointer
- moveReadPointerIfNeeded(writePointer);
}
+if (!isInvalid) {
+ if (checkpointWritePointers != null) {
--- End diff --
good point. done
---
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 #23: [Tephra-201] Store checkpoints in in-prog...
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/23#discussion_r91457499
--- Diff:
tephra-core/src/main/java/org/apache/tephra/TransactionManager.java ---
@@ -348,27 +348,30 @@ private void cleanupTimedOutTransactions() {
}
long currentTime = System.currentTimeMillis();
-List timedOut = Lists.newArrayList();
+Map timedOut = Maps.newHashMap();
--- End diff --
Unrelated to the change, but it seems like the locking in this method is
wrong.
First, we are holding the read lock for the entire cleanup method, which
seems unnecessary. The inProgress itself is a concurrent tree, so for the first
`for-loop`, it shouldn't need the lock at all, as it is just for collecting the
in progress tx that are timed out.
Then in the `if (!timedOut.isEmpty())`, seems like only the modification to
`invalid` and `invalidArray` needs to be protected by lock, but I think it
should be a "write lock"?
---
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.
---
