[GitHub] incubator-tephra issue #47: [TEPHRA-240] Include conflicting key and client ...

[anew]

Github user anew commented on the issue:

https://github.com/apache/incubator-tephra/pull/47
  
squashed and rebased on latest master.


---

[jira] [Commented] (TEPHRA-240) TransactionConflictException should contain the conflicting key and client id

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TEPHRA-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16163826#comment-16163826
 ] 

ASF GitHub Bot commented on TEPHRA-240:
---

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", 
1);
 
-//
+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. 


> TransactionConflictException should contain the conflicting key and client id
> -
>
> Key: TEPHRA-240
> URL: https://issues.apache.org/jira/browse/TEPHRA-240
> Project: Tephra
>  Issue Type: Bug
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[anew]

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", 
1);
 
-//
+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. 


---

[jira] [Commented] (TEPHRA-240) TransactionConflictException should contain the conflicting key and client id

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TEPHRA-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16163803#comment-16163803
 ] 

ASF GitHub Bot commented on TEPHRA-240:
---

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", 
1);
 
-//
+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.


> TransactionConflictException should contain the conflicting key and client id
> -
>
> Key: TEPHRA-240
> URL: https://issues.apache.org/jira/browse/TEPHRA-240
> Project: Tephra
>  Issue Type: Bug
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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 changes) throws 
(1:TTransactionNotInProgressException e),
-  TBoolean canCommitOrThrow(1: TTransaction tx, 2: set 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 changes) throws 
(1: TTransactionNotInProgressException e),
+  void canCommitOrThrow(1: i64 tx, 2: set 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.


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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", 
1);
 
-//
+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.


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[anew]

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

https://github.com/apache/incubator-tephra/pull/47#discussion_r138437903
  
--- 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 changeIds)
-throws TransactionNotInProgressException, TransactionSizeException {
+  public void canCommit(long txId, Collection 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())) {
+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 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 --

Good catch. I forgot to remove this from existing code. 


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[anew]

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

https://github.com/apache/incubator-tephra/pull/47#discussion_r138437669
  
--- 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 changeIds) 
throws TransactionFailureException;
+  void canCommitOrThrow(Transaction tx, Collection changeIds)
+throws TransactionNotInProgressException, 
TransactionConflictException, TransactionSizeException;
--- End diff --

Replaced for canCommitOrThrow() and commitOrThrow(). 


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[anew]

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

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

it does not make a difference, but may be more future-proof.


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[anew]

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

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

ok


---

[GitHub] incubator-tephra pull request #56: (TEPHRA-258) Improve logging in thrift cl...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[jira] [Commented] (TEPHRA-240) TransactionConflictException should contain the conflicting key and client id

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TEPHRA-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16162616#comment-16162616
 ] 

ASF GitHub Bot commented on TEPHRA-240:
---

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 changeIds)
-throws TransactionNotInProgressException, TransactionSizeException {
+  public void canCommit(long txId, Collection 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())) {
+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 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`.


> TransactionConflictException should contain the conflicting key and client id
> -
>
> Key: TEPHRA-240
> URL: https://issues.apache.org/jira/browse/TEPHRA-240
> Project: Tephra
>  Issue Type: Bug
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TEPHRA-240) TransactionConflictException should contain the conflicting key and client id

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TEPHRA-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16162615#comment-16162615
 ] 

ASF GitHub Bot commented on TEPHRA-240:
---

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.


> TransactionConflictException should contain the conflicting key and client id
> -
>
> Key: TEPHRA-240
> URL: https://issues.apache.org/jira/browse/TEPHRA-240
> Project: Tephra
>  Issue Type: Bug
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TEPHRA-240) TransactionConflictException should contain the conflicting key and client id

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TEPHRA-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16162614#comment-16162614
 ] 

ASF GitHub Bot commented on TEPHRA-240:
---

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 changeIds) 
throws TransactionFailureException;
+  void canCommitOrThrow(Transaction tx, Collection 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?


> TransactionConflictException should contain the conflicting key and client id
> -
>
> Key: TEPHRA-240
> URL: https://issues.apache.org/jira/browse/TEPHRA-240
> Project: Tephra
>  Issue Type: Bug
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (TEPHRA-240) TransactionConflictException should contain the conflicting key and client id

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/TEPHRA-240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16162617#comment-16162617
 ] 

ASF GitHub Bot commented on TEPHRA-240:
---

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?


> TransactionConflictException should contain the conflicting key and client id
> -
>
> Key: TEPHRA-240
> URL: https://issues.apache.org/jira/browse/TEPHRA-240
> Project: Tephra
>  Issue Type: Bug
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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?


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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 changeIds) 
throws TransactionFailureException;
+  void canCommitOrThrow(Transaction tx, Collection 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?


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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.


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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 changeIds)
-throws TransactionNotInProgressException, TransactionSizeException {
+  public void canCommit(long txId, Collection 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())) {
+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 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`.


---

[GitHub] incubator-tephra pull request #47: [TEPHRA-240] Include conflicting key and ...

[poornachandra]

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`.


---

[jira] [Resolved] (TEPHRA-258) Improve log message for Thrift client connection

[Andreas Neumann (JIRA)]

 [ 
https://issues.apache.org/jira/browse/TEPHRA-258?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Andreas Neumann resolved TEPHRA-258.

Resolution: Fixed

> Improve log message for Thrift client connection
> 
>
> Key: TEPHRA-258
> URL: https://issues.apache.org/jira/browse/TEPHRA-258
> Project: Tephra
>  Issue Type: Improvement
>Reporter: Andreas Neumann
>Assignee: Andreas Neumann
> Fix For: 0.13.0-incubating
>
>
> The current message reads:
> {noformat}
> 2017-08-01 11:56:35,568 - INFO  [streams-worker-thread-25:?@?] - Attempting 
> to connect to tx service at : with timeout 3 ms.
> {noformat}
> This timeout is easy to confuse with the transaction timeout, but it is the 
> Thrift RPC timeout.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)