Create a custom thrift protocol to ensure client/server compatibility #5398
Conversation
|
Created this in draft since, for now, it only addresses the task outlined in the ticket:
I'm not sure if any of the other proposed tasks should be added here or as follow-on PRs. |
| public static class AccumuloProtocol extends TCompactProtocol { | ||
|
|
||
| private static final int MAGIC_NUMBER = 0x41434355; // "ACCU" in ASCII | ||
| private static final byte PROTOCOL_VERSION = 1; |
There was a problem hiding this comment.
Should probably add a comment to AccumuloDataVersion.CURRENT_VERSION that references this and mentions that when changing CURRENT_VERSION may also need to change this.
There was a problem hiding this comment.
I was wondering about that... whether the protocol version should be independent, so we can version the serialization format of the protocol header.
We will probably want to include the AccumuloDataVersion.CURRENT_VERSION in the protocol header directly, though. We could also include the actual Accumulo release version in here, too (major.minor.patch; e.g. 4.0.0). I think to verify, we'd probably want to ensure the same major and same minor version, if we want to be even more restrictive than the data version. We really shouldn't have Accumulo servers or clients/servers talking across major.minor versions (different patch releases are okay, and is important for rolling upgrades).
There was a problem hiding this comment.
I left it separate for now just in case we wanted to have multiple protocol versions able to talk to each other but am not sure about that.
The data version and protocol version definitely seem related but I'm not sure how closely tied we want to have those versions.
| try { | ||
| this.writeHeader(); | ||
| } catch (TException e) { | ||
| if (scope != null) { |
There was a problem hiding this comment.
span has a recordException message, but not sure if that should be used here. I have seen it but have not looked into it in detail. Might be worth looking into.
| } | ||
| } | ||
|
|
||
| super.writeMessageBegin(message); |
There was a problem hiding this comment.
Seems like this should be inside the try block above. Thinking if this message throws exception that writeMessageEnd will never be called. If it should be in the try block then maybe the existing code in 2.1 has a bug.
There was a problem hiding this comment.
I'm not sure I understand. Putting super.writeMessageBegin(message); in the try-catch would close the span and scope if an exception is thrown from it but I'm not sure that it would make writeMessageEnd() in either case, right? Maybe im missing something here.
There was a problem hiding this comment.
If super.writeMessageBegin(message) throws an exception, then we can probably assume that writeMessageEnd() will never be called. So if this happens will nothing close the span and scope?
| if (this.isClient && scope != null) { | ||
| scope.close(); | ||
| span.end(); | ||
| } |
There was a problem hiding this comment.
I don't think we care if it's a client. If a scope was opened, we'd want to close it.
|
|
||
| @Override | ||
| public void writeMessageBegin(TMessage message) throws TException { | ||
| if (this.isClient) { |
There was a problem hiding this comment.
The code this replaces used the same protocol for both clients and servers. I'm not 100% sure when writeMessageBegin gets called, but I think it's probably fine to do it on every message, and not discriminate based on client and server.
There was a problem hiding this comment.
Yea I am kinda unsure about this point (not sure whats correct/better).
If I understand things correctly here, the thrift messages flow in both directions between "client" and "server" over the same connection. So as long as we validate compatibility when the client sends a message to the server, than we can avoid doing the extra work of adding (and therefore validating) the headers when the server responds to the client.
I think things would work just fine if we did not distinguish between client and server in this context, we would just be doing more work than necessary which might be unoptimal (which seems like it matters in this context).
I'm also thinking though there might be the benefit of always writing and validating these header just so we are certain that there are no edge cases or scenarios that I'm not thinking about where a server responds to a client over a connection that has not been validated yet. I don't know if this would ever happen but it seems like a trade off between absolute certainty that we have valid comms both ways and optimizing the amount of work done (again might be worth the optimization in this case, not sure).
Definitely would appreciate feedback on this line of thinking and corrections regarding any incorrect assumptions ive made above.
There was a problem hiding this comment.
If I understand things correctly here, the thrift messages flow in both directions between "client" and "server" over the same connection. So as long as we validate compatibility when the client sends a message to the server, than we can avoid doing the extra work of adding (and therefore validating) the headers when the server responds to the client.
That sounds good to me. It seems like the changes in the code to support this are minimal, do you know if you have caught everything in the code for covering this functionality?
Addresses #5390