TEZ-4008: Pluggable AM FrameworkServices and AmExtensions (2/3) #426
Conversation
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
This comment was marked as outdated.
This comment was marked as outdated.
ff0acb6 to
fa6ec35
Compare
This comment was marked as outdated.
This comment was marked as outdated.
23ce3eb to
f0a349c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
325bcf4 to
e08ff20
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e08ff20 to
8febe5b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
8febe5b to
5a2a6d3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
5a2a6d3 to
6ff2aa5
Compare
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
reviewed
Sep 3, 2025
Member
ayushtkn
left a comment
There was a problem hiding this comment.
Thanx @abstractdog, minor comments, rest LGTM
tez-api/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.hadoop</groupId> | ||
| <artifactId>hadoop-yarn-registry</artifactId> |
Member
There was a problem hiding this comment.
use hadoop-registry, hadoop-yarn-registry is just a stub pom, which has only hadoop-registry
https://issues.apache.org/jira/browse/HADOOP-15821
Comment on lines
49
to
61
| public AMRecord(AMRecord other) { | ||
| this.appId = other.getApplicationId(); | ||
| this.host = other.getHost(); | ||
| this.port = other.getPort(); | ||
| this.id = other.getId(); | ||
| } | ||
|
|
||
| public AMRecord(ServiceRecord serviceRecord) { | ||
| this.appId = ApplicationId.fromString(serviceRecord.get(APP_ID_RECORD_KEY)); | ||
| this.host = serviceRecord.get(HOST_RECORD_KEY); | ||
| this.port = Integer.parseInt(serviceRecord.get(PORT_RECORD_KEY)); | ||
| this.id = serviceRecord.get(OPAQUE_ID_KEY); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
they aren't used from tez code base, instead at tez clients that might want to handle tez unmanaged sessions
I know this looks strange at first sight, but let me add javadoc clarifying this, and you'll see if it's more straightforward
|
|
||
| @Override | ||
| public boolean equals(Object other) { | ||
| if (other instanceof AMRecord otherRecord) { |
Member
There was a problem hiding this comment.
should we bail out early if other == this?
Contributor
Author
There was a problem hiding this comment.
yeah, that branch is part of traditional equals method, let me add
| } | ||
| } | ||
|
|
||
| public ServiceRecord toServiceRecord() { |
Contributor
Author
There was a problem hiding this comment.
same as above, adding javadoc
Comment on lines
100
to
102
| @Override | ||
| public int hashCode() { | ||
| return appId.hashCode() * host.hashCode() * id.hashCode() + port; |
Member
There was a problem hiding this comment.
multiplying can easily lead to overflow & two values can collide easily. If any of appId, host or id is null, this will lead to NPE as well.
How about using
return Objects.hash(appId, host, port, id);
Contributor
Author
There was a problem hiding this comment.
nice, I haven't used this before :D let me fix
| public static final String TEZ_CLIENT_VERSION_ENV = "TEZ_CLIENT_VERSION"; | ||
|
|
||
| //Arbitrary opaque ID to identify AM instances from AMRegistryClient | ||
| public static final String TEZ_AM_UUID = "UUID"; |
Member
There was a problem hiding this comment.
is there a possibility to use TEZ_AM_UUID rather than just UUID?
| Configuration conf = new Configuration(); | ||
| AMRegistry amRegistry = AMRegistryUtils.createAMRegistry(conf); | ||
| assertNull(amRegistry); | ||
| String className = "org.apache.tez.dag.api.client.registry.TestAMRegistry$SkeletonAMRegistry"; |
Member
There was a problem hiding this comment.
can we do String className = SkeletonAMRegistry.class.getName(); rather than hardcoding
Comment on lines
50
to
51
| amRegistry = AMRegistryUtils.createAMRegistry(conf); | ||
| assertEquals(className, amRegistry.getClass().getName()); |
Member
There was a problem hiding this comment.
should add
assertNotNull(amRegistry);
Else if it still stays null, rather than failing with some nice error, it will fail with NPE
Contributor
Author
There was a problem hiding this comment.
good catch, adding it
86e29f2 to
a0159bd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
thanks for the review so far, @ayushtkn, addressed them, precommit is green |
a0159bd to
b05793a
Compare
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
As described on jira:
Interface and reflective plugin configuration for a pool of AMs identified by a namespace.
The registry should allow each DAGClientServer to register/unregister themselves from a pool.
Basically, this PR introduces AMRegistry that is going to be implemented in TEZ-4007 by a Zookeeper AM registry.