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 2015/11/30 21:18:51 UTC
[GitHub] accumulo pull request: ACCUMULO-4065 Create a separate connection ...
GitHub user joshelser opened a pull request:
https://github.com/apache/accumulo/pull/54
ACCUMULO-4065 Create a separate connection cache for oneway Thrift ca…
…lls.
Oneway Thrift calls return to the caller before the response from a server
is received. Even though all oneway calls are void methods, there is still
an implicit response sent by the server so that the client knows when
the RPC has completed. Because of this potential delay, a connection that
is re-used by a non-oneway call could see the response from the oneway
call, not its own (void or typed response message). We must separate
connections to ensure that this is avoided.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/joshelser/accumulo ACCUMULO-4065-oneway-isolation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/54.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 #54
----
commit 55e05fc11c04cae42dbba9b3ec1d6138b9bfd85b
Author: Josh Elser <el...@apache.org>
Date: 2015-11-27T23:27:24Z
ACCUMULO-4065 Create a separate connection cache for oneway Thrift calls.
Oneway Thrift calls return to the caller before the response from a server
is received. Even though all oneway calls are void methods, there is still
an implicit response sent by the server so that the client knows when
the RPC has completed. Because of this potential delay, a connection that
is re-used by a non-oneway call could see the response from the oneway
call, not its own (void or typed response message). We must separate
connections to ensure that this is avoided.
----
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160761763
> I have been able to verify that what we were doing does not work.
Can you expand on this? I am really curious about the behavior of thrift when a oneway throws an exception on the server 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: ACCUMULO-4065 Create a separate connection ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160989204
For reference. The code @joshelser was talking about [Thrift 0.9.1 ProcessFunction line 45](https://github.com/apache/thrift/blob/0.9.1/lib/java/src/org/apache/thrift/ProcessFunction.java#L45) and [Accumulo 1.7.0 RpcWrapper line 51](https://github.com/apache/accumulo/blob/1.7.0/server/base/src/main/java/org/apache/accumulo/server/rpc/RpcWrapper.java#L51)
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160785269
Interesting. `#2` should not occur in Accumulo because the connection should be reserved. `#4` seems like it could occur though. For `#3` what do you meant when you say `both threads see exceptions`? Which threads? I wouldn't think the thread executing the one way would ever see anything, assuming it never waits for a response. Can you post your test code?
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160787497
One thing I was curious about in your test code was if you used some synchronization mechanism to force the ordering of something like `B, A, B', A'` vs `B, A, A', B'`.
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160801600
@keith-turner and I have been talking in IRC about these changes. After staring at this for so long, I think we've come to a realization that the underlying issue is related to Exceptions in oneway calls. We can see that the base ProcessFunction class in Thrift will write a message back to the client when it sees a TException from the Processor implementation (the server-side implementation of our Thrift server).
The problem is that, in trying to work around the semantics change in THRIFT-1805, we end up creating a TException which causes the server to write a message over the wire back to clients, even in oneway calls. While, it's arguable that a TException is ever written back to clients for oneway calls, I think fixing RpcWrapper to not wrap RuntimeExceptions and Errors as TExceptions would be a fix for us (cc\ @ctubbsii).
Closing this out as I think it's unnecessary and going to try to address it via reflection in RpcWrapper.
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser closed the pull request at:
https://github.com/apache/accumulo/pull/54
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160762876
> Can you expand on this? I am really curious about the behavior of thrift when a oneway throws an exception on the server side.
Best as I understand it: the oneway modifier just equates to the generated code not calling the method to read the response for an RPC off the wire. This has multiple implications:
1. The caller does not know if the application received the message (only that the network connection succeeded).
2. The caller does still receive a response object (this is how void works though, not oneway)
3. If you try to run another RPC that isn't oneway on the same connection, it's possible that the synchronous call will get goofed up by that oneway's void response (this might be a Thrift bug -- it's certainly a pain to work around).
Let me test what actually happens in the client code when the oneway throws an exception. I have a little test harness that I used to play around with 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 pull request: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160772542
> Let me test what actually happens in the client code when the oneway throws an exception. I have a little test harness that I used to play around with this.
So this is funny :). Let's define some stuff:
* Given a request A with response A' over a "normal" synchronous thrift call
* Given a request B with response B' over a oneway thrift call
* Both of these requests are submitted over the same connection
* A is run in one thread, B in another (with a latch to ensure they start at the same time)
1. If A is not submitted and the computation of B' in the server throws an exception, the caller of B does not see this exception. Only side-effect is a server-side log message.
2. If the order of execution as seen by the client is `A, B, B', A'`, the client will typically get an exception trying to read A'
3. If the order of execution as seen by the client is `B, A, A', B'`, then both threads see exceptions.
4. If the order of execution as seen by the client is `B, A, B', A'`, then an exception is seen reading A'
Does this help, @keith-turner?
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160793220
> #2 should not occur in Accumulo because the connection should be reserved.
Yup, that's right.
> For #3 what do you meant when you say both threads see exceptions
Per the test setup: two threads are used, one to run A and the other to run B. Another way of saying running both A and B saw exceptions.
The code I was using: https://github.com/joshelser/noway-thrift
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160751007
Just started a full run of the ITs on my jenkins too.
---
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: ACCUMULO-4065 Create a separate connection ...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/54#issuecomment-160749591
As far as testing goes, I haven't been able to recreate the original situation inside of Accumulo. I have been able to verify that what we were doing does not work. I've done some testing locally found some bugs along the way. I am presently running some CI, and will try to kick off a CI run against some >1 nodes later today.
---
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.
---