You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by joshelser <gi...@git.apache.org> on 2016/09/15 00:25:36 UTC
[GitHub] accumulo pull request #152: ACCUMULO-4456 Make NotActiveServiceException an ...
GitHub user joshelser opened a pull request:
https://github.com/apache/accumulo/pull/152
ACCUMULO-4456 Make NotActiveServiceException an RTE
This would naturally be happening anyways because this
exception is not caught by the Thrift processor. This should
bubble up to the client and the client will naturally retry
the call it was going to make, including a re-lookup of
the master location.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/joshelser/accumulo 4456-rte-switch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/152.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 #152
----
commit 06b680d95fc7f84e81eb5c59f2888c80b75a28cf
Author: Josh Elser <el...@apache.org>
Date: 2016-09-15T00:24:01Z
ACCUMULO-4456 Make NotActiveServiceException an RTE
This would naturally be happening anyways because this
exception is not caught by the Thrift processor. This should
bubble up to the client and the client will naturally retry
the call it was going to make, including a re-lookup of
the master location.
----
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/accumulo/pull/152
> Sounds like we'd have to make it a TException then?
I don't think this will work. But this assertion is based on a fuzzy memory of some code in the past doing that and it not working that way. However, I am not certain about this.
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/accumulo/pull/152
I think this looks good @joshelser
---
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] accumulo pull request #152: ACCUMULO-4456 Make NotActiveServiceException an ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/152
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/accumulo/pull/152
>The testing I did was unable to force a failure, but I am not convinced if this is because I didn't figure out how to test it good enough.
This seems really hard to test. One way I can think to test is to write some code for a one time test.
* Start a master where active is always false. Do this via a one time code modification.
* Write some code that calls the internal methods to connect to that master and make a thrift call.
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/152
Thanks for the prompt review, Keith. Will go ahead and squash+merge.
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/accumulo/pull/152
@joshelser have you tested this? Does it cause a retry? I suspect it will not cause a retry, but not completely sure. I think the NotActiveServiceException may cause the client to see a TApplicationException which clients do not retry on, because clients treat this as an unexpected server side exception.
---
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] accumulo pull request #152: ACCUMULO-4456 Make NotActiveServiceException an ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/152#discussion_r79067584
--- Diff: server/base/src/main/java/org/apache/accumulo/server/rpc/NotActiveServiceException.java ---
@@ -19,7 +19,7 @@
/**
* An {@link Exception} which denotes that the service which was invoked is not the active instance for that service in the Accumulo cluster.
*/
-public class NotActiveServiceException extends Exception {
+public class NotActiveServiceException extends RuntimeException {
--- End diff --
Is still still used?
---
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] accumulo pull request #152: ACCUMULO-4456 Make NotActiveServiceException an ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/152#discussion_r79066218
--- Diff: core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java ---
@@ -247,10 +252,14 @@ private void executeFateOperation(long opid, FateOperation op, List<ByteBuffer>
try {
client = MasterClient.getConnectionWithRetry(context);
client.executeFateOperation(Tracer.traceInfo(), context.rpcCreds(), opid, op, args, opts, autoCleanUp);
- break;
+ return;
} catch (TTransportException tte) {
log.debug("Failed to call executeFateOperation(), retrying ... ", tte);
sleepUninterruptibly(100, TimeUnit.MILLISECONDS);
+ } catch (ThriftNotActiveServiceException e) {
--- End diff --
could almost use the new java convention where catch can list multiple exceptions, except log msgs differ
---
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] accumulo pull request #152: ACCUMULO-4456 Make NotActiveServiceException an ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/152#discussion_r79069091
--- Diff: server/base/src/main/java/org/apache/accumulo/server/rpc/NotActiveServiceException.java ---
@@ -19,7 +19,7 @@
/**
* An {@link Exception} which denotes that the service which was invoked is not the active instance for that service in the Accumulo cluster.
*/
-public class NotActiveServiceException extends Exception {
+public class NotActiveServiceException extends RuntimeException {
--- End diff --
its already removed
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/152
Ok, turns out I had a bug :). I was able to reproduce this and it does cause an AccumuloException to be thrown on the client side.
---
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] accumulo pull request #152: ACCUMULO-4456 Make NotActiveServiceException an ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/152#discussion_r79069134
--- Diff: server/base/src/main/java/org/apache/accumulo/server/rpc/NotActiveServiceException.java ---
@@ -19,7 +19,7 @@
/**
* An {@link Exception} which denotes that the service which was invoked is not the active instance for that service in the Accumulo cluster.
*/
-public class NotActiveServiceException extends Exception {
+public class NotActiveServiceException extends RuntimeException {
--- End diff --
Nope, this is nuked in fb1232f
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/152
> have you tested this? Does it cause a retry?
So, I tried to test this with via SIGSTOP'ing a client application, flipping the active master, and then SIGCONT'ing the application and I was unable to get the client to actually die without this change. Reading the docs, we would have been throwing a RuntimeException anyways (because the checked exception is not capable of being thrown by our thrift RPC methods), so java would have wrapped it in a UndeclaredThrowableException.
> I suspect it will not cause a retry, but not completely sure
Sounds like we'd have to make it a TException then? I went digging in Thrift code last night trying to remember how this all works...
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/152
> This seems really hard to test. One way I can think to test is to write some code for a one time test.
:) yeah, something like this is what I was trying to avoid but will probably have to do this today to actually check it. Oh well.
---
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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/152
@keith-turner can you 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] accumulo issue #152: ACCUMULO-4456 Make NotActiveServiceException an RTE
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/152
> Reading the docs, we would have been throwing a RuntimeException anyways (because the checked exception is not capable of being thrown by our thrift RPC methods), so java would have wrapped it in a UndeclaredThrowableException.
I don't think this is 100% clear. What the code does at runtime would be equivalent to what this patch changes (this patch is just more clear). The testing I did was unable to force a failure, but I am not convinced if this is because I didn't figure out how to test it good enough.
---
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.
---