Uploaded image for project: 'Tephra'
  1. Tephra
  2. TEPHRA-240

TransactionConflictException should contain the conflicting key and client id

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.13.0-incubating
    • Component/s: None
    • Labels:
      None

      Description

      Often transaction conflicts are hard to explain. Having the conflicting key, or even the name of the client that performed the concurrent update would greatly help debug.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tephra/pull/47
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anew commented on the issue:

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

          squashed and rebased on latest master.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on the issue: https://github.com/apache/incubator-tephra/pull/47 squashed and rebased on latest master.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138490048

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -206,12 +210,19 @@ public TransactionManager(Configuration conf, @Nonnull TransactionStateStorage p
          // TODO: REMOVE WITH txnBackwardsCompatCheck()
          longTimeoutTolerance = conf.getLong("data.tx.long.timeout.tolerance", 10000);

          • //
            + ClientIdRetention retention = ClientIdRetention.valueOf(
              • End diff –

          Hmmm. We don't do that for the other configurations... if any of the numeric properties cannot be parsed as a number, it also fails. I think it is a good idea to fail on invalid configuration, because if there is a configuration that is present, then that is most likely with the intention to change the default. If there is a typo or some other error, and we only log a warning, that warning is likely to go unnoticed and the system will behave in a way that was intended, and that will go undetected until it causes failures.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138490048 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -206,12 +210,19 @@ public TransactionManager(Configuration conf, @Nonnull TransactionStateStorage p // TODO: REMOVE WITH txnBackwardsCompatCheck() longTimeoutTolerance = conf.getLong("data.tx.long.timeout.tolerance", 10000); // + ClientIdRetention retention = ClientIdRetention.valueOf( End diff – Hmmm. We don't do that for the other configurations... if any of the numeric properties cannot be parsed as a number, it also fails. I think it is a good idea to fail on invalid configuration, because if there is a configuration that is present, then that is most likely with the intention to change the default. If there is a typo or some other error, and we only log a warning, that warning is likely to go unnoticed and the system will behave in a way that was intended, and that will go undetected until it causes failures.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138489380

          — Diff: tephra-core/src/main/thrift/transaction.thrift —
          @@ -73,15 +79,21 @@ service TTransactionServer {
          // TODO remove this as it was replaced with startShortWithTimeout in 0.10
          TTransaction startShortTimeout(1: i32 timeout),
          TTransaction startShortClientId(1: string clientId) throws (1: TGenericException e),

          • TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1:TGenericException e),
          • TTransaction startShortWithTimeout(1: i32 timeout) throws (1:TGenericException e),
          • TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e),
          • TBoolean canCommitOrThrow(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e,
          • 2:TGenericException g,),
            + TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1: TGenericException e),
            + TTransaction startShortWithTimeout(1: i32 timeout) throws (1: TGenericException e),
            + // TODO remove this as it was replaced with canCommitOrThrow in 0.13
            + TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e),
            + void canCommitOrThrow(1: i64 tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e,
            + 2: TTransactionConflictException c,
            + 3: TGenericException g),
            + // TODO remove this as it was replaced with commitWithExn in 0.13
            TBoolean commitTx(1: TTransaction tx) throws (1:TTransactionNotInProgressException e),
            + void commitOrThrow(1: i64 txId, 2: i64 wp) throws (1: TTransactionNotInProgressException e,
              • End diff –

          Can't do that. Thrift does not support exception hierarchies. I did add the TGenericException such that we can transport other exception types in the future.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anew commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138489380 — Diff: tephra-core/src/main/thrift/transaction.thrift — @@ -73,15 +79,21 @@ service TTransactionServer { // TODO remove this as it was replaced with startShortWithTimeout in 0.10 TTransaction startShortTimeout(1: i32 timeout), TTransaction startShortClientId(1: string clientId) throws (1: TGenericException e), TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1:TGenericException e), TTransaction startShortWithTimeout(1: i32 timeout) throws (1:TGenericException e), TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e), TBoolean canCommitOrThrow(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e, 2:TGenericException g,), + TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1: TGenericException e), + TTransaction startShortWithTimeout(1: i32 timeout) throws (1: TGenericException e), + // TODO remove this as it was replaced with canCommitOrThrow in 0.13 + TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e), + void canCommitOrThrow(1: i64 tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e, + 2: TTransactionConflictException c, + 3: TGenericException g), + // TODO remove this as it was replaced with commitWithExn in 0.13 TBoolean commitTx(1: TTransaction tx) throws (1:TTransactionNotInProgressException e), + void commitOrThrow(1: i64 txId, 2: i64 wp) throws (1: TTransactionNotInProgressException e, End diff – Can't do that. Thrift does not support exception hierarchies. I did add the TGenericException such that we can transport other exception types in the future.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138480498

          — Diff: tephra-core/src/main/thrift/transaction.thrift —
          @@ -73,15 +79,21 @@ service TTransactionServer {
          // TODO remove this as it was replaced with startShortWithTimeout in 0.10
          TTransaction startShortTimeout(1: i32 timeout),
          TTransaction startShortClientId(1: string clientId) throws (1: TGenericException e),

          • TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1:TGenericException e),
          • TTransaction startShortWithTimeout(1: i32 timeout) throws (1:TGenericException e),
          • TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e),
          • TBoolean canCommitOrThrow(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e,
          • 2:TGenericException g,),
            + TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1: TGenericException e),
            + TTransaction startShortWithTimeout(1: i32 timeout) throws (1: TGenericException e),
            + // TODO remove this as it was replaced with canCommitOrThrow in 0.13
            + TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e),
            + void canCommitOrThrow(1: i64 tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e,
            + 2: TTransactionConflictException c,
            + 3: TGenericException g),
            + // TODO remove this as it was replaced with commitWithExn in 0.13
            TBoolean commitTx(1: TTransaction tx) throws (1:TTransactionNotInProgressException e),
            + void commitOrThrow(1: i64 txId, 2: i64 wp) throws (1: TTransactionNotInProgressException e,
              • End diff –

          I missed it yesterday, it would be good to use TransactionFailureException here too for future-proofing.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138480498 — Diff: tephra-core/src/main/thrift/transaction.thrift — @@ -73,15 +79,21 @@ service TTransactionServer { // TODO remove this as it was replaced with startShortWithTimeout in 0.10 TTransaction startShortTimeout(1: i32 timeout), TTransaction startShortClientId(1: string clientId) throws (1: TGenericException e), TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1:TGenericException e), TTransaction startShortWithTimeout(1: i32 timeout) throws (1:TGenericException e), TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e), TBoolean canCommitOrThrow(1: TTransaction tx, 2: set<binary> changes) throws (1:TTransactionNotInProgressException e, 2:TGenericException g,), + TTransaction startShortWithClientIdAndTimeOut(1: string clientId, 2: i32 timeout) throws (1: TGenericException e), + TTransaction startShortWithTimeout(1: i32 timeout) throws (1: TGenericException e), + // TODO remove this as it was replaced with canCommitOrThrow in 0.13 + TBoolean canCommitTx(1: TTransaction tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e), + void canCommitOrThrow(1: i64 tx, 2: set<binary> changes) throws (1: TTransactionNotInProgressException e, + 2: TTransactionConflictException c, + 3: TGenericException g), + // TODO remove this as it was replaced with commitWithExn in 0.13 TBoolean commitTx(1: TTransaction tx) throws (1:TTransactionNotInProgressException e), + void commitOrThrow(1: i64 txId, 2: i64 wp) throws (1: TTransactionNotInProgressException e, End diff – I missed it yesterday, it would be good to use TransactionFailureException here too for future-proofing.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138477861

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -206,12 +210,19 @@ public TransactionManager(Configuration conf, @Nonnull TransactionStateStorage p
          // TODO: REMOVE WITH txnBackwardsCompatCheck()
          longTimeoutTolerance = conf.getLong("data.tx.long.timeout.tolerance", 10000);

          • //
            + ClientIdRetention retention = ClientIdRetention.valueOf(
              • End diff –

          It would be good to catch the exception during enum conversion and use the default value. This way transaction manager will still startup on a misconfiguration.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138477861 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -206,12 +210,19 @@ public TransactionManager(Configuration conf, @Nonnull TransactionStateStorage p // TODO: REMOVE WITH txnBackwardsCompatCheck() longTimeoutTolerance = conf.getLong("data.tx.long.timeout.tolerance", 10000); // + ClientIdRetention retention = ClientIdRetention.valueOf( End diff – It would be good to catch the exception during enum conversion and use the default value. This way transaction manager will still startup on a misconfiguration.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138222710

          — Diff: tephra-api/src/main/java/org/apache/tephra/TransactionConflictException.java —
          @@ -22,11 +22,50 @@

          • Thrown to indicate transaction conflict occurred when trying to commit a transaction.
            */
            public class TransactionConflictException extends TransactionFailureException {
            +
            + /**
            + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)} instead.
            + */
            + @Deprecated
            public TransactionConflictException(String message) { super(message); + transactionId = null; + conflictingChange = null; + conflictingClient = null; }

            + /**
            + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)}

            instead.
            + */
            + @Deprecated
            public TransactionConflictException(String message, Throwable cause)

            { super(message, cause); + transactionId = null; + conflictingChange = null; + conflictingClient = null; + }

            +
            + public TransactionConflictException(long transactionId, String conflictingChange, String conflictingClient) {

              • End diff –

          I think `conflictingKey` would be a more appropriate name for `conflictingChange`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138222710 — Diff: tephra-api/src/main/java/org/apache/tephra/TransactionConflictException.java — @@ -22,11 +22,50 @@ Thrown to indicate transaction conflict occurred when trying to commit a transaction. */ public class TransactionConflictException extends TransactionFailureException { + + /** + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)} instead. + */ + @Deprecated public TransactionConflictException(String message) { super(message); + transactionId = null; + conflictingChange = null; + conflictingClient = null; } + /** + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)} instead. + */ + @Deprecated public TransactionConflictException(String message, Throwable cause) { super(message, cause); + transactionId = null; + conflictingChange = null; + conflictingClient = null; + } + + public TransactionConflictException(long transactionId, String conflictingChange, String conflictingClient) { End diff – I think `conflictingKey` would be a more appropriate name for `conflictingChange`.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138238770

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionContext.java —
          @@ -311,25 +304,16 @@ private void persist() throws TransactionFailureException {
          }

          private void commit() throws TransactionFailureException {

          • boolean commitSuccess = false;
            try { - commitSuccess = txClient.commit(currentTx); - }

            catch (TransactionNotInProgressException e)

            { - String message = String.format("Transaction %d is not in progress.", currentTx.getTransactionId()); - LOG.warn(message, e); - abort(new TransactionFailureException(message, e)); - // abort will throw that exception + txClient.commitOrThrow(currentTx); + }

            catch (TransactionNotInProgressException | TransactionConflictException e) {

              • End diff –

          This should also catch `TransactionFailureException` like the catch block of method `checkForConflicts()` above, right?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138238770 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionContext.java — @@ -311,25 +304,16 @@ private void persist() throws TransactionFailureException { } private void commit() throws TransactionFailureException { boolean commitSuccess = false; try { - commitSuccess = txClient.commit(currentTx); - } catch (TransactionNotInProgressException e) { - String message = String.format("Transaction %d is not in progress.", currentTx.getTransactionId()); - LOG.warn(message, e); - abort(new TransactionFailureException(message, e)); - // abort will throw that exception + txClient.commitOrThrow(currentTx); + } catch (TransactionNotInProgressException | TransactionConflictException e) { End diff – This should also catch `TransactionFailureException` like the catch block of method `checkForConflicts()` above, right?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138270921

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -853,46 +867,45 @@ private void advanceWritePointer(long writePointer) {
          }
          }

          • public boolean canCommit(Transaction tx, Collection<byte[]> changeIds)
          • throws TransactionNotInProgressException, TransactionSizeException {
            + public void canCommit(long txId, Collection<byte[]> changeIds)
            + throws TransactionNotInProgressException, TransactionSizeException, TransactionConflictException {

          txMetricsCollector.rate("canCommit");
          Stopwatch timer = new Stopwatch().start();

          • InProgressTx inProgressTx = inProgress.get(tx.getTransactionId());
            + InProgressTx inProgressTx = inProgress.get(txId);
            if (inProgressTx == null) {
            synchronized (this) {
            // invalid transaction, either this has timed out and moved to invalid, or something else is wrong.
          • if (invalidTxList.contains(tx.getTransactionId()))
            Unknown macro: { + if (invalidTxList.contains(txId)) { throw new TransactionNotInProgressException( String.format( - "canCommit() is called for transaction %d that is not in progress (it is known to be invalid)", - tx.getTransactionId())); + "canCommit() is called for transaction %d that is not in progress (it is known to be invalid)", txId)); } else { throw new TransactionNotInProgressException( - String.format("canCommit() is called for transaction %d that is not in progress", tx.getTransactionId())); + String.format("canCommit() is called for transaction %d that is not in progress", txId)); } }

            }

          Set<ChangeId> set =

          • validateChangeSet(tx, changeIds, inProgressTx.clientId != null ? inProgressTx.clientId : DEFAULT_CLIENTID);
            -
          • if (hasConflicts(tx, set)) {
          • return false;
            + validateChangeSet(txId, changeIds, inProgressTx.clientId != null ? inProgressTx.clientId : DEFAULT_CLIENTID);
            + for (byte[] change : changeIds) {
              • End diff –

          I don't think this for-loop is needed as method `validateChangeSet()` already returns `Set<ChangeId>`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138270921 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -853,46 +867,45 @@ private void advanceWritePointer(long writePointer) { } } public boolean canCommit(Transaction tx, Collection<byte[]> changeIds) throws TransactionNotInProgressException, TransactionSizeException { + public void canCommit(long txId, Collection<byte[]> changeIds) + throws TransactionNotInProgressException, TransactionSizeException, TransactionConflictException { txMetricsCollector.rate("canCommit"); Stopwatch timer = new Stopwatch().start(); InProgressTx inProgressTx = inProgress.get(tx.getTransactionId()); + InProgressTx inProgressTx = inProgress.get(txId); if (inProgressTx == null) { synchronized (this) { // invalid transaction, either this has timed out and moved to invalid, or something else is wrong. if (invalidTxList.contains(tx.getTransactionId())) Unknown macro: { + if (invalidTxList.contains(txId)) { throw new TransactionNotInProgressException( String.format( - "canCommit() is called for transaction %d that is not in progress (it is known to be invalid)", - tx.getTransactionId())); + "canCommit() is called for transaction %d that is not in progress (it is known to be invalid)", txId)); } else { throw new TransactionNotInProgressException( - String.format("canCommit() is called for transaction %d that is not in progress", tx.getTransactionId())); + String.format("canCommit() is called for transaction %d that is not in progress", txId)); } } } Set<ChangeId> set = validateChangeSet(tx, changeIds, inProgressTx.clientId != null ? inProgressTx.clientId : DEFAULT_CLIENTID); - if (hasConflicts(tx, set)) { return false; + validateChangeSet(txId, changeIds, inProgressTx.clientId != null ? inProgressTx.clientId : DEFAULT_CLIENTID); + for (byte[] change : changeIds) { End diff – I don't think this for-loop is needed as method `validateChangeSet()` already returns `Set<ChangeId>`.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138237949

          — Diff: tephra-api/src/main/java/org/apache/tephra/TransactionConflictException.java —
          @@ -22,11 +22,50 @@

          • Thrown to indicate transaction conflict occurred when trying to commit a transaction.
            */
            public class TransactionConflictException extends TransactionFailureException {
            +
            + /**
            + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)} instead.
            + */
            + @Deprecated
            public TransactionConflictException(String message) { super(message); + transactionId = null; + conflictingChange = null; + conflictingClient = null; }

            + /**
            + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)}

            instead.
            + */
            + @Deprecated
            public TransactionConflictException(String message, Throwable cause)

            { super(message, cause); + transactionId = null; + conflictingChange = null; + conflictingClient = null; + }

            +
            + public TransactionConflictException(long transactionId, String conflictingChange, String conflictingClient)

            { + super(String.format("Transaction %d conflicts with %s on change key '%s'", transactionId, + conflictingClient == null ? "unknown client" : conflictingClient, conflictingChange)); + this.transactionId = transactionId; + this.conflictingChange = conflictingChange; + this.conflictingClient = conflictingClient; + }

            +
            + private final Long transactionId;

              • End diff –

          It would be good if the fields are defined before the constructor definitions.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138237949 — Diff: tephra-api/src/main/java/org/apache/tephra/TransactionConflictException.java — @@ -22,11 +22,50 @@ Thrown to indicate transaction conflict occurred when trying to commit a transaction. */ public class TransactionConflictException extends TransactionFailureException { + + /** + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)} instead. + */ + @Deprecated public TransactionConflictException(String message) { super(message); + transactionId = null; + conflictingChange = null; + conflictingClient = null; } + /** + * @deprecated since 0.13-incubating. Use {@link #TransactionConflictException(long, String, String)} instead. + */ + @Deprecated public TransactionConflictException(String message, Throwable cause) { super(message, cause); + transactionId = null; + conflictingChange = null; + conflictingClient = null; + } + + public TransactionConflictException(long transactionId, String conflictingChange, String conflictingClient) { + super(String.format("Transaction %d conflicts with %s on change key '%s'", transactionId, + conflictingClient == null ? "unknown client" : conflictingClient, conflictingChange)); + this.transactionId = transactionId; + this.conflictingChange = conflictingChange; + this.conflictingClient = conflictingClient; + } + + private final Long transactionId; End diff – It would be good if the fields are defined before the constructor definitions.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/incubator-tephra/pull/47#discussion_r138239467

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java —
          @@ -89,21 +89,38 @@
          *

          • @param tx transaction to verify
          • @param changeIds ids of changes made by transaction
          • * @return true if transaction can be committed otherwise false
          • * @throws TransactionSizeException if the size of the chgange set exceeds the allowed limit
          • * @throws TransactionNotInProgressException if the transaction is not in progress; most likely it has timed out.
            + *
            + * @throws TransactionSizeException if the size of the change set exceeds the allowed limit
            + * @throws TransactionConflictException if the change set has a conflict with an overlapping transaction
            + * @throws TransactionNotInProgressException if the transaction is not in progress; most likely it has timed out
            */
          • boolean canCommitOrThrow(Transaction tx, Collection<byte[]> changeIds) throws TransactionFailureException;
            + void canCommitOrThrow(Transaction tx, Collection<byte[]> changeIds)
            + throws TransactionNotInProgressException, TransactionConflictException, TransactionSizeException;
              • End diff –

          Would declaring `TransactionFailureException` here help in making the API resilient to some future changes - in-case we need to add new exceptions to indicate a transaction failure?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/47#discussion_r138239467 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java — @@ -89,21 +89,38 @@ * @param tx transaction to verify @param changeIds ids of changes made by transaction * @return true if transaction can be committed otherwise false * @throws TransactionSizeException if the size of the chgange set exceeds the allowed limit * @throws TransactionNotInProgressException if the transaction is not in progress; most likely it has timed out. + * + * @throws TransactionSizeException if the size of the change set exceeds the allowed limit + * @throws TransactionConflictException if the change set has a conflict with an overlapping transaction + * @throws TransactionNotInProgressException if the transaction is not in progress; most likely it has timed out */ boolean canCommitOrThrow(Transaction tx, Collection<byte[]> changeIds) throws TransactionFailureException; + void canCommitOrThrow(Transaction tx, Collection<byte[]> changeIds) + throws TransactionNotInProgressException, TransactionConflictException, TransactionSizeException; End diff – Would declaring `TransactionFailureException` here help in making the API resilient to some future changes - in-case we need to add new exceptions to indicate a transaction failure?
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user anew opened a pull request:

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

          TEPHRA-240 Include conflicting key and client id in TransactionConflictException

          • Enhances TransactionConflictException to have fields for the transaction id, the conflicting change, and the conflicting client id
          • TransactionManager to retain the client id for change sets after the transaction has committed
          • A configuration for whether and how long it should be retained. This is to allow disabling this feature (and it effect on memory)
          • TransactionManager.(can)Commit now throws TrandactionConflictException instead of returning a boolean
          • new thrift methods (and a new thrift exception). The existing thrift methods will remain backward-compatible
          • change TransactionSystemClient and TransactionContext to use the new methods
          • new test case (and existing test case changes) for this
          • a few unrelated warnings have been removed from TransactionManager
          • a bit of test case refactoring to remove duplication of code

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

          $ git pull https://github.com/anew/incubator-tephra tephra-240

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

          https://github.com/apache/incubator-tephra/pull/47.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 #47


          commit 205714fdd126ab7537f9f66e6e156c9736f54646
          Author: anew <anew@apache.org>
          Date: 2017-08-17T23:21:52Z

          TEPHRA-240 Include conflicting key and client id in TransactionConflictException


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user anew opened a pull request: https://github.com/apache/incubator-tephra/pull/47 TEPHRA-240 Include conflicting key and client id in TransactionConflictException Enhances TransactionConflictException to have fields for the transaction id, the conflicting change, and the conflicting client id TransactionManager to retain the client id for change sets after the transaction has committed A configuration for whether and how long it should be retained. This is to allow disabling this feature (and it effect on memory) TransactionManager.(can)Commit now throws TrandactionConflictException instead of returning a boolean new thrift methods (and a new thrift exception). The existing thrift methods will remain backward-compatible change TransactionSystemClient and TransactionContext to use the new methods new test case (and existing test case changes) for this a few unrelated warnings have been removed from TransactionManager a bit of test case refactoring to remove duplication of code You can merge this pull request into a Git repository by running: $ git pull https://github.com/anew/incubator-tephra tephra-240 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tephra/pull/47.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 #47 commit 205714fdd126ab7537f9f66e6e156c9736f54646 Author: anew <anew@apache.org> Date: 2017-08-17T23:21:52Z TEPHRA-240 Include conflicting key and client id in TransactionConflictException
          Hide
          anew Andreas Neumann added a comment -

          Currently, we do not keep the client id after the transaction commits. The change set that is kept for conflict detection is not labeled with the client id. Adding that is easy but may increase the memory footprint. Therefore, there should be an configuration option whether this should be kept or not.

          Also, because the client id is only in memory but not persisted to the transaction snapshot, this info will not be available after a restart.

          Show
          anew Andreas Neumann added a comment - Currently, we do not keep the client id after the transaction commits. The change set that is kept for conflict detection is not labeled with the client id. Adding that is easy but may increase the memory footprint. Therefore, there should be an configuration option whether this should be kept or not. Also, because the client id is only in memory but not persisted to the transaction snapshot, this info will not be available after a restart.

            People

            • Assignee:
              anew Andreas Neumann
              Reporter:
              anew Andreas Neumann
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development