Uh oh!
There was an error while loading. Please reload this page.
TEZ-4708: TezTaskUmbilicalProtocol to use a protobuf-based protocol - #483
Open
Aggarwal-Raghav wants to merge 2 commits into
Open
TEZ-4708: TezTaskUmbilicalProtocol to use a protobuf-based protocol#483Aggarwal-Raghav wants to merge 2 commits into
Aggarwal-Raghav wants to merge 2 commits into
Conversation
tez-yetus
commented
Apr 16, 2026
💔 -1 overall
This message was automatically generated. |
steveloughran
left a comment
There was a problem hiding this comment.
very fast implementation of the core design.
I do think the payloads of each message should be proto messages, rather than wrapped Writables, as that's more in line with what proto RPCs are meant to do, and hopefully more robust.
what you do have is already better than today and will handle the removal of WritableRPCEngine
| option java_generic_services = true; | ||
| option java_generate_equals_and_hash = true; | ||
| message GetTaskRequestProto { |
There was a problem hiding this comment.
ideally the protocol should be marshalling the fields in the proto message, rather than just serializing the writable and unmarshalling.
ContributorAuthor
There was a problem hiding this comment.
@steveloughran , can you please provide feedback on the updated TezTaskUmbilicalProtocol.proto. Because of structured mapping vs Wrapped Writable (old approach), the lineage of messages that needs to be defined has increased 😅
TezTaskAttemptID → TezTaskID → TezVertexID → TezDAGID → ApplicationId (Hadoop) → clusterTimestamp (long) + appId (int)
In MAPREDUCE-6706, its still Wrapped Writable as in the top comment in TaskUmbilicalProtocol.proto Could you confirm if the level of granularity I've introduced here is what you had in mind for Tez? I want to ensure I've not over-complicated the .proto definition.
Writable-serialized types are carried as opaque bytes, for example jvm_context and jvm_task
| * limitations under the License. | ||
| */ | ||
| syntax = "proto2"; |
Contributor
There was a problem hiding this comment.
we can go for proto3 I believe
| /** Called by the Task process to retrieve the work from the AM. */ | ||
| @Override | ||
| public ContainerTask getTask(ContainerContext containerContext) throws IOException { | ||
| return service( |
ContributorAuthor
There was a problem hiding this comment.
Used the same the service and translate FunctionalInterfaces for Exception wrapping as in MR
| protocolName = "org.apache.tez.runtime.internals.protocolPB.TezTaskUmbilicalProtocolBlockingPB", | ||
| protocolVersion = 1) | ||
| public interface TezTaskUmbilicalProtocolBlockingPB | ||
| extends TezTaskUmbilicalProtocol.BlockingInterface, VersionedProtocol {} |
ContributorAuthor
There was a problem hiding this comment.
Have added VersionedProtocol which is additional compared to MR
ContributorAuthor
There was a problem hiding this comment.
the java class naming is kept to be inline with DAGClientAMProtocolBlockingPB
tez-yetus
commented
Apr 19, 2026
💔 -1 overall
This message was automatically generated. |
ContributorAuthor
|
Aggarwal-Raghav
commented
Apr 21, 2026
ContributorAuthor
Interim status: investigating the UT failures, the underlying reason appears to be same. The ProtoRPCEngine2 is using  |
ok, ProtoRPCEngine uses protobuf 2.5 public and is frozen if you want to use our 3.x version (currently 3.25)
I'd recommend you do that so no audits of dependencies flag up protobuf 2.5 jars. Recent hadoop releases (3.4.3+) do not bundle these |
Aggarwal-Raghav
commented
May 1, 2026
ContributorAuthor
Once TEZ-4715 is merged, will rebase again. Wanted to check if UT are fixed in CI (locally they are passing) and spotbugs shouldn't flag generated proto code. |
Aggarwal-Raghav
marked this pull request as ready for review
May 1, 2026 07:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.