Create a custom thrift protocol to ensure client/server compatibility #5398

DomGarguilo · 2025-03-11T20:06:46Z

Addresses #5390

DomGarguilo · 2025-03-11T20:08:26Z

Created this in draft since, for now, it only addresses the task outlined in the ticket:

create a custom protocol with the versioning information stored in a header and checks on the server side to validate it

I'm not sure if any of the other proposed tasks should be added here or as follow-on PRs.

keith-turner · 2025-03-11T23:29:52Z

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java

+  public static class AccumuloProtocol extends TCompactProtocol {
+
+    private static final int MAGIC_NUMBER = 0x41434355; // "ACCU" in ASCII
+    private static final byte PROTOCOL_VERSION = 1;


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.

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

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.

I think the protocol version can be independently numbered, not connected to the data version. I think it's sufficient to check the Accumulo version (Constants.VERSION) major.minor. We don't need to use the data version in here at all. The protocol version is the version of the RPC protocol... the data version is the version of the data persisted in ZK and HDFS... and the Accumulo version is the overall version of the software. Only the RPC protocol version and the Accumulo version are relevant here.

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java

ctubbsii

I'm not sure if we actually need to distinguish between client and server for the spans.

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java

DomGarguilo · 2025-03-17T20:57:30Z

Marking this as ready for review. I tried to address the following two suggestions from the ticket:

Primarily, create a custom protocol with the versioning information stored in a header and checks on the server side to validate it, then

Secondarily, simplify the thrift APIs by adding tracing info to the protocol header

There are a lot of changed files in this PR since I had to update the thrift code. There are also a lot of places where the only change is the removal of the TInfo object as the first param in a method call which was a trivial change. Hopefully that helps skip/skim over portions of the diff while reviewing.

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java

core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java

DomGarguilo · 2025-03-28T18:43:55Z

@kevinrr888 your suggestions should all be addressed as of 29c1234

in that commit I...

refactored the protocol version compatibility check method to throw an exception directly instead of return a boolean. I also updated the wording on that message to make things more clear
addressed the IDE warnings regarding nullness in various places
added javadoc to the static client and server factory methods in ThriftUtil to make it clearer which should be used

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java

core/src/test/java/org/apache/accumulo/core/rpc/AccumuloProtocolTest.java

ctubbsii · 2025-04-16T00:56:04Z

One additional thought I had was that clients should send the instanceId, and the server should verify it, so that we don't get connections from clients for a different instance.

I was trying to work this in but am not sure the best place to pull in the instanceId from. I tried to pass it in as a parameter but had to reach pretty high up the chain which changed a lot of code. Was hoping you had a better idea for where I can add this in from.

The ServerContext and/or ClientContext should be able to provide the InstanceId, and the protocol factory could be constructed with it. I'm not sure how much code that touches. I can try to take a look if you hit a wall here.

core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java

…olTest.java Co-authored-by: Christopher Tubbs <[email protected]>

Co-authored-by: Christopher Tubbs <[email protected]

DomGarguilo · 2025-04-16T19:48:53Z

One additional thought I had was that clients should send the instanceId, and the server should verify it, so that we don't get connections from clients for a different instance.

I was trying to work this in but am not sure the best place to pull in the instanceId from. I tried to pass it in as a parameter but had to reach pretty high up the chain which changed a lot of code. Was hoping you had a better idea for where I can add this in from.

The ServerContext and/or ClientContext should be able to provide the InstanceId, and the protocol factory could be constructed with it. I'm not sure how much code that touches. I can try to take a look if you hit a wall here.

Okay I added the instance ID validation in 769d38f. Had to reach pretty far up the chain to get the instanceId so there were quite a few files changed but all of those are just parameter changes.

ctubbsii · 2025-04-23T20:02:29Z

Can you provide an example of the opentelemetry tracing output (screenshot or log sequences or something)? I'd be curious what they look like when a client performs a traced operation over the new RPC protocol.

DomGarguilo · 2025-04-23T22:53:01Z

Can you provide an example of the opentelemetry tracing output (screenshot or log sequences or something)? I'd be curious what they look like when a client performs a traced operation over the new RPC protocol.

I'm not too sure how much this shows but here is a screenshot from the "graph view" in Jaeger capturing traces from a scan from the shell

keith-turner · 2025-06-06T20:10:29Z

@DomGarguilo and @ctubbsii curious what the status of this PR is. Would be nice to get this merged in if possible.

DomGarguilo · 2025-06-06T20:36:17Z

@DomGarguilo and @ctubbsii curious what the status of this PR is. Would be nice to get this merged in if possible.

Ready to be merged unless anyone else has blockers here.

ctubbsii · 2025-06-07T05:03:18Z

I think this is probably ready to be merged, but I'd like to go over the visual with @DomGarguilo to make sure I understand what I'm looking at and everything is working as it should. Sorry for holding this up. I think it can be merged next week. @DomGarguilo could you give me a call next week to go over it first, if you have time?

DomGarguilo · 2025-06-13T14:27:28Z

I am actually not sure that traces are being propagated correctly. I'm looking into things but I don't think this should be merged yet.

DomGarguilo · 2025-06-13T18:02:50Z

I am actually not sure that traces are being propagated correctly. I'm looking into things but I don't think this should be merged yet.

After debugging. The issue is fixed via a683811 and I can see spans from client -> server in Jaeger. The issue was that we were not closing the scope or span on AccumuloProtocol.readMessageEnd() so the traces were never closed on the server side, and thus those server spans never finished (and couldn’t be linked back to the client spans in Jaeger).

In the screenshot below, in a shell I ran trace on then scan (on an existing table with an entry) then trace off.

This demonstrates that cross-component tracing is working.

ctubbsii

Looking at the code again today, after that fix for the readMessageEnd...

I think the stuff with the version checks, and all that is all good, and we should definitely merge that stuff in soon. And, I really wanted the changes to the tracing stuff also, because I was really hoping it would allow us to simplify a lot of the Thrift code.

However, I'm starting to wonder if pushing the tracing stuff into this is actually feasible. When I looked at it again today, I think I'm starting to realize that readMessageEnd might finish before the actual action that the RPC was requesting is executed. If that's the case, then the span is already closed, and any subsequent spans on the server side are going to be disconnected. That seems to match what I'm seeing in the second screenshot, where we only see the handleRpcMessage span, but no child spans for it on the server.

The current code creates a span that wraps the entire execution of the RPC call, but I think in the protocol, it's only wrapping the message parsing, not the execution. I am not too familiar with Thrift internals, but I suspect a subsequent writeMessage is done on the server side to send a message back to the client with the results of the execution. If so, we really want to close the handleRpcMessage span at that time, and not before.

It may still be possible to salvage these tracing changes, and close the span at the end of the RPC execution inside the protocol factory. However, if that's not possible, we may have to abandon the thrift tracing changes, and continue doing that the way it is currently done, so it wraps the whole RPC operation, and only keep the portions that do the version/compatibility checks.

Create a custom thrift protocol to ensure client/server compatibility

2ddf5ae

DomGarguilo added this to the 4.0.0 milestone Mar 11, 2025

DomGarguilo requested a review from ctubbsii March 11, 2025 20:06

DomGarguilo self-assigned this Mar 11, 2025

Fix checkstyle violation

d2083db

keith-turner reviewed Mar 11, 2025

View reviewed changes

ctubbsii reviewed Mar 12, 2025

View reviewed changes

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java Outdated Show resolved Hide resolved

DomGarguilo added 8 commits March 12, 2025 14:03

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

46e2bac

Address feedback

1ea49cc

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

00f74e0

add exception recording to the span

5dfab38

remove TInfo and use headers instead

5f01ee0

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

3c89fef

make header map final

8022a49

remove missed TInfo use

17b418c

DomGarguilo marked this pull request as ready for review March 17, 2025 20:57

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

5e4c62f

keith-turner reviewed Mar 19, 2025

View reviewed changes

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java Outdated Show resolved Hide resolved

DomGarguilo added 4 commits March 19, 2025 16:59

Add unit test

4b8460f

Use int instead of short

59a55ae

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

7f1d05f

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

a52ebd5

DomGarguilo linked an issue Mar 20, 2025 that may be closed by this pull request

Create a custom thrift protocol to ensure client/server compatibility #5390

Open

kevinrr888 reviewed Mar 26, 2025

View reviewed changes

Address feedback

29c1234

kevinrr888 reviewed Mar 28, 2025

View reviewed changes

core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java Outdated Show resolved Hide resolved

Use TriftUtil helper method for exception tracing

473426d

DomGarguilo added 2 commits April 15, 2025 15:40

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

f1dff97

Create inner child span for each RPC

8d742ea

ctubbsii reviewed Apr 16, 2025

View reviewed changes

core/src/test/java/org/apache/accumulo/core/rpc/AccumuloProtocolTest.java Outdated Show resolved Hide resolved

ctubbsii reviewed Apr 16, 2025

View reviewed changes

core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java Outdated Show resolved Hide resolved

DomGarguilo and others added 5 commits April 16, 2025 11:03

Update core/src/test/java/org/apache/accumulo/core/rpc/AccumuloProtoc…

adc8e26

…olTest.java Co-authored-by: Christopher Tubbs <[email protected]>

Remove unneeded null check

aa5c8f5

Co-authored-by: Christopher Tubbs <[email protected]

Add instance ID validation to header

769d38f

use getInstanceId() method

cee2e7f

fix checkstyle

c93077b

DomGarguilo requested a review from ctubbsii April 16, 2025 19:49

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

f3c0fab

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

4bbdf9d

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

2b11f7d

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

177f016

DomGarguilo added 2 commits June 13, 2025 13:57

Add override for readMessageEnd, closing span

a683811

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

528d3d1

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

6bf3c4b

ctubbsii reviewed Jun 26, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into accumuloProtocol

8e4eb78

DomGarguilo mentioned this pull request Jun 30, 2025

Implement custom Thrift protocol to ensure client/server compatibility #5691

Open

DomGarguilo marked this pull request as draft June 30, 2025 18:30

Create a custom thrift protocol to ensure client/server compatibility #5398

Are you sure you want to change the base?

Create a custom thrift protocol to ensure client/server compatibility #5398

Uh oh!

Conversation

DomGarguilo commented Mar 11, 2025

Uh oh!

DomGarguilo commented Mar 11, 2025

Uh oh!

keith-turner Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

DomGarguilo Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

ctubbsii Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DomGarguilo commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DomGarguilo commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

ctubbsii commented Apr 16, 2025

Uh oh!

Uh oh!

DomGarguilo commented Apr 16, 2025

Uh oh!

ctubbsii commented Apr 23, 2025

Uh oh!

DomGarguilo commented Apr 23, 2025

Uh oh!

keith-turner commented Jun 6, 2025

Uh oh!

DomGarguilo commented Jun 6, 2025

Uh oh!

ctubbsii commented Jun 7, 2025

Uh oh!

DomGarguilo commented Jun 13, 2025

Uh oh!

DomGarguilo commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DomGarguilo commented Jun 13, 2025 •

edited

Loading