You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tephra.apache.org by anew <gi...@git.apache.org> on 2016/10/20 00:07:01 UTC
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
GitHub user anew opened a pull request:
https://github.com/apache/incubator-tephra/pull/18
[TEPHRA-194] Make startShort() throw IllegalArgumentException \u2026
\u2026 consistently for invalid timeout values.
Previously, it was throwing IllegalArgumentException in-memory and Exception for thrift mode.
This breaks down into:
- adding a new exception to the thrift stubs to be able to communicate back any exception
- adding a new method startShortWithTimeout that throws that exception
- modifying the TransactionServiceThriftClient to convert that exception back into an IllegalArgumentException
- modifying the TransactionSystemTest to expect IllegalArgumentException now
- modifying the ThriftTransactionServerTest to use a retry strategy that counts retries and validate that the number of retries is 0 if startShort() throws IllegalArgumentException
- modifying TransactionServiceClient to accept a class name as the retry strategy provider (needed for above test case)
- some unrelated but reasonable style fixes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/anew/incubator-tephra tephra-194
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tephra/pull/18.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 #18
----
commit 64be9f46b8d3cc823d9eb303f9eb8ed7811375c7
Author: anew <an...@apache.org>
Date: 2016-10-19T23:59:33Z
[TEPHRA-194] Make startShort() throw IllegalArgumentException consistently for invalid timeout values
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tephra/pull/18
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by poornachandra <gi...@git.apache.org>.
Github user poornachandra commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84387594
--- Diff: tephra-core/src/test/java/org/apache/tephra/TransactionSystemTest.java ---
@@ -33,25 +33,33 @@
*/
public abstract class TransactionSystemTest {
- public static final byte[] C1 = new byte[] { 'c', '1' };
- public static final byte[] C2 = new byte[] { 'c', '2' };
- public static final byte[] C3 = new byte[] { 'c', '3' };
- public static final byte[] C4 = new byte[] { 'c', '4' };
+ private static final byte[] C1 = new byte[] { 'c', '1' };
+ private static final byte[] C2 = new byte[] { 'c', '2' };
+ private static final byte[] C3 = new byte[] { 'c', '3' };
+ private static final byte[] C4 = new byte[] { 'c', '4' };
protected abstract TransactionSystemClient getClient() throws Exception;
protected abstract TransactionStateStorage getStateStorage() throws Exception;
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testNegativeTimeout() throws Exception {
- getClient().startShort(-1);
+ try {
+ getClient().startShort(-1);
+ Assert.fail("Expected illegal argument for negative timeout");
+ } catch (IllegalArgumentException e) {
--- End diff --
What if some other exception is thrown? Would be good to use `@Test(expected = IllegalArgumentException.class)`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84392263
--- Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceThriftClient.java ---
@@ -112,7 +113,20 @@ public Transaction startShort() throws TException {
public Transaction startShort(int timeout) throws TException {
try {
- return TransactionConverterUtils.unwrap(client.startShortTimeout(timeout));
+ return TransactionConverterUtils.unwrap(client.startShortWithTimeout(timeout));
+ } catch (TGenericException e) {
+ isValid.set(false);
+ // currently, we only expect IllegalArgumentException here, if the timeout is invalid
+ IllegalArgumentException actual;
+ try {
+ actual = (IllegalArgumentException)
+ Class.forName(e.getOriginalExceptionClass())
+ .getConstructor(String.class)
+ .newInstance(e.getMessage());
+ } catch (Throwable t) {
--- End diff --
Because we do not expect anything else than IllegalArgumentException? For now, I will change the code to be more readable and expect exactly that.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84390730
--- Diff: tephra-core/src/test/java/org/apache/tephra/TransactionSystemTest.java ---
@@ -33,25 +33,33 @@
*/
public abstract class TransactionSystemTest {
- public static final byte[] C1 = new byte[] { 'c', '1' };
- public static final byte[] C2 = new byte[] { 'c', '2' };
- public static final byte[] C3 = new byte[] { 'c', '3' };
- public static final byte[] C4 = new byte[] { 'c', '4' };
+ private static final byte[] C1 = new byte[] { 'c', '1' };
+ private static final byte[] C2 = new byte[] { 'c', '2' };
+ private static final byte[] C3 = new byte[] { 'c', '3' };
+ private static final byte[] C4 = new byte[] { 'c', '4' };
protected abstract TransactionSystemClient getClient() throws Exception;
protected abstract TransactionStateStorage getStateStorage() throws Exception;
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testNegativeTimeout() throws Exception {
- getClient().startShort(-1);
+ try {
+ getClient().startShort(-1);
+ Assert.fail("Expected illegal argument for negative timeout");
+ } catch (IllegalArgumentException e) {
+ // expected
+ }
}
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testExcessiveTimeout() throws Exception {
- getClient().startShort((int) TimeUnit.DAYS.toSeconds(10));
+ try {
+ getClient().startShort((int) TimeUnit.DAYS.toSeconds(10));
+ Assert.fail("Expected illegal argument for excessive timeout");
+ } catch (IllegalArgumentException e) {
--- End diff --
same as above
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by poornachandra <gi...@git.apache.org>.
Github user poornachandra commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84388540
--- Diff: tephra-core/src/test/java/org/apache/tephra/ThriftTransactionSystemTest.java ---
@@ -124,4 +127,39 @@ protected TransactionSystemClient getClient() throws Exception {
protected TransactionStateStorage getStateStorage() throws Exception {
return storage;
}
+
+ @Override
+ public void testNegativeTimeout() throws Exception {
+ CountingRetryStrategyProvider.retries.set(0);
+ super.testNegativeTimeout();
+ Assert.assertEquals(0, CountingRetryStrategyProvider.retries.get());
--- End diff --
It would be good to add a comment on why the retries should be zero 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra issue #18: [TEPHRA-194] Make startShort() throw IllegalArgu...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on the issue:
https://github.com/apache/incubator-tephra/pull/18
Please do not review Thrift classes :)
tephra-core/src/main/java/org/apache/tephra/distributed/thrift/T*.java
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra issue #18: [TEPHRA-194] Make startShort() throw IllegalArgu...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on the issue:
https://github.com/apache/incubator-tephra/pull/18
@poornachandra I have addressed your comments, please take a look.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84391234
--- Diff: tephra-core/src/test/java/org/apache/tephra/ThriftTransactionSystemTest.java ---
@@ -124,4 +127,39 @@ protected TransactionSystemClient getClient() throws Exception {
protected TransactionStateStorage getStateStorage() throws Exception {
return storage;
}
+
+ @Override
+ public void testNegativeTimeout() throws Exception {
+ CountingRetryStrategyProvider.retries.set(0);
+ super.testNegativeTimeout();
+ Assert.assertEquals(0, CountingRetryStrategyProvider.retries.get());
--- End diff --
good point, added.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by poornachandra <gi...@git.apache.org>.
Github user poornachandra commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84387667
--- Diff: tephra-core/src/test/java/org/apache/tephra/TransactionSystemTest.java ---
@@ -33,25 +33,33 @@
*/
public abstract class TransactionSystemTest {
- public static final byte[] C1 = new byte[] { 'c', '1' };
- public static final byte[] C2 = new byte[] { 'c', '2' };
- public static final byte[] C3 = new byte[] { 'c', '3' };
- public static final byte[] C4 = new byte[] { 'c', '4' };
+ private static final byte[] C1 = new byte[] { 'c', '1' };
+ private static final byte[] C2 = new byte[] { 'c', '2' };
+ private static final byte[] C3 = new byte[] { 'c', '3' };
+ private static final byte[] C4 = new byte[] { 'c', '4' };
protected abstract TransactionSystemClient getClient() throws Exception;
protected abstract TransactionStateStorage getStateStorage() throws Exception;
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testNegativeTimeout() throws Exception {
- getClient().startShort(-1);
+ try {
+ getClient().startShort(-1);
+ Assert.fail("Expected illegal argument for negative timeout");
+ } catch (IllegalArgumentException e) {
+ // expected
+ }
}
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testExcessiveTimeout() throws Exception {
- getClient().startShort((int) TimeUnit.DAYS.toSeconds(10));
+ try {
+ getClient().startShort((int) TimeUnit.DAYS.toSeconds(10));
+ Assert.fail("Expected illegal argument for excessive timeout");
+ } catch (IllegalArgumentException e) {
--- End diff --
Same 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by poornachandra <gi...@git.apache.org>.
Github user poornachandra commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84386647
--- Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceThriftClient.java ---
@@ -112,7 +113,20 @@ public Transaction startShort() throws TException {
public Transaction startShort(int timeout) throws TException {
try {
- return TransactionConverterUtils.unwrap(client.startShortTimeout(timeout));
+ return TransactionConverterUtils.unwrap(client.startShortWithTimeout(timeout));
+ } catch (TGenericException e) {
+ isValid.set(false);
+ // currently, we only expect IllegalArgumentException here, if the timeout is invalid
+ IllegalArgumentException actual;
+ try {
+ actual = (IllegalArgumentException)
+ Class.forName(e.getOriginalExceptionClass())
+ .getConstructor(String.class)
+ .newInstance(e.getMessage());
+ } catch (Throwable t) {
--- End diff --
Would be good to log `t` in trace over 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84390716
--- Diff: tephra-core/src/test/java/org/apache/tephra/TransactionSystemTest.java ---
@@ -33,25 +33,33 @@
*/
public abstract class TransactionSystemTest {
- public static final byte[] C1 = new byte[] { 'c', '1' };
- public static final byte[] C2 = new byte[] { 'c', '2' };
- public static final byte[] C3 = new byte[] { 'c', '3' };
- public static final byte[] C4 = new byte[] { 'c', '4' };
+ private static final byte[] C1 = new byte[] { 'c', '1' };
+ private static final byte[] C2 = new byte[] { 'c', '2' };
+ private static final byte[] C3 = new byte[] { 'c', '3' };
+ private static final byte[] C4 = new byte[] { 'c', '4' };
protected abstract TransactionSystemClient getClient() throws Exception;
protected abstract TransactionStateStorage getStateStorage() throws Exception;
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testNegativeTimeout() throws Exception {
- getClient().startShort(-1);
+ try {
+ getClient().startShort(-1);
+ Assert.fail("Expected illegal argument for negative timeout");
+ } catch (IllegalArgumentException e) {
--- End diff --
This is because in ThriftTransactionSystemTest, we want to assert additionally that the number of retries is 0. That cannot be done if the test case has (expected=IllegalArgumentException.class).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84193226
--- Diff: tephra-core/src/main/java/org/apache/tephra/distributed/thrift/TBoolean.java ---
@@ -24,19 +24,29 @@
*/
package org.apache.tephra.distributed.thrift;
-import org.apache.thrift.EncodingUtils;
--- End diff --
Note that thrift does not generate imports in the order that the checkstyle rules expect. The previous version of the generated thrift stubs was apparently manually changed to reflect the correct order and to have a copyright. For the copyright, I added an instruction how to add that to the README file, but for import order, we really should not fix that manually every time we run thrift. So we see a lot of diffs in the import section 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra pull request #18: [TEPHRA-194] Make startShort() throw Ille...
Posted by anew <gi...@git.apache.org>.
Github user anew commented on a diff in the pull request:
https://github.com/apache/incubator-tephra/pull/18#discussion_r84399721
--- Diff: tephra-core/src/test/java/org/apache/tephra/TransactionSystemTest.java ---
@@ -33,25 +33,33 @@
*/
public abstract class TransactionSystemTest {
- public static final byte[] C1 = new byte[] { 'c', '1' };
- public static final byte[] C2 = new byte[] { 'c', '2' };
- public static final byte[] C3 = new byte[] { 'c', '3' };
- public static final byte[] C4 = new byte[] { 'c', '4' };
+ private static final byte[] C1 = new byte[] { 'c', '1' };
+ private static final byte[] C2 = new byte[] { 'c', '2' };
+ private static final byte[] C3 = new byte[] { 'c', '3' };
+ private static final byte[] C4 = new byte[] { 'c', '4' };
protected abstract TransactionSystemClient getClient() throws Exception;
protected abstract TransactionStateStorage getStateStorage() throws Exception;
- // Unfortunately, in-memory mode and thrift mode throw different exceptions here
- @Test(expected = Exception.class)
+ @Test
public void testNegativeTimeout() throws Exception {
- getClient().startShort(-1);
+ try {
+ getClient().startShort(-1);
+ Assert.fail("Expected illegal argument for negative timeout");
+ } catch (IllegalArgumentException e) {
--- End diff --
I don't think you can. If I annotate the base class method with (expected=...), then that is expected also for the sub class method. By the way, I don't think there is a problem with coupling between the tests. They are testing the same thing on different implementations, only that one of them wants to make an additional assertion
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tephra issue #18: [TEPHRA-194] Make startShort() throw IllegalArgu...
Posted by poornachandra <gi...@git.apache.org>.
Github user poornachandra commented on the issue:
https://github.com/apache/incubator-tephra/pull/18
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---