TEZ-4008: Pluggable AM FrameworkServices and AmExtensions (2/3) by abstractdog · Pull Request #426 · apache/tez

abstractdog · 2025-09-02T08:11:26Z

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.

tez-yetus · 2025-09-03T03:53:43Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 11s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+0 🆗	xmllint	0m 0s		xmllint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 3 new or modified test files.
			_ master Compile Tests _
+0 🆗	mvndep	1m 55s		Maven dependency ordering for branch
+1 💚	mvninstall	8m 13s		master passed
+1 💚	compile	0m 55s		master passed
+1 💚	checkstyle	0m 52s		master passed
+1 💚	javadoc	0m 53s		master passed
+0 🆗	spotbugs	1m 36s		tez-api in master has 610 extant spotbugs warnings.
+0 🆗	spotbugs	0m 59s		tez-dag in master has 785 extant spotbugs warnings.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 6s		Maven dependency ordering for patch
+1 💚	mvninstall	0m 36s		the patch passed
+1 💚	codespell	0m 15s		No new issues.
+1 💚	compile	0m 37s		the patch passed
+1 💚	javac	0m 37s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 17s		the patch passed
+1 💚	javadoc	0m 25s		the patch passed
+1 💚	spotbugs	1m 54s		the patch passed
			_ Other Tests _
+1 💚	unit	1m 49s		tez-api in the patch passed.
+1 💚	unit	4m 8s		tez-dag in the patch passed.
+1 💚	asflicense	0m 17s		The patch does not generate ASF License warnings.
		27m 4s

Subsystem	Report/Notes
Docker	ClientAPI=1.51 ServerAPI=1.51 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-426/9/artifact/out/Dockerfile
GITHUB PR	#426
Optional Tests	dupname asflicense codespell detsecrets xmllint javac javadoc unit compile spotbugs checkstyle
uname	Linux 6f32f1b38442 5.15.0-143-generic #153-Ubuntu SMP Fri Jun 13 19:10:45 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-home/workspace/tez-multibranch_PR-426/src/.yetus/personality.sh
git revision	master / `6ff2aa5`
Default Java	Ubuntu-21.0.8+9-Ubuntu-0ubuntu124.04.1
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-426/9/testReport/
Max. process+thread count	382 (vs. ulimit of 5500)
modules	C: tez-api tez-dag U: .
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-426/9/console
versions	git=2.43.0 maven=3.8.7 spotbugs=4.9.3 codespell=2.0.0
Powered by	Apache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

ayushtkn

Thanx @abstractdog, minor comments, rest LGTM

ayushtkn · 2025-09-03T14:24:15Z

 </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-yarn-registry</artifactId>


use hadoop-registry, hadoop-yarn-registry is just a stub pom, which has only hadoop-registry
https://issues.apache.org/jira/browse/HADOOP-15821

ayushtkn · 2025-09-03T14:25:42Z

+ 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);
+ }


these two aren't used anywhere

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

ayushtkn · 2025-09-03T14:27:44Z

+
+ @Override
+ public boolean equals(Object other) {
+ if (other instanceof AMRecord otherRecord) {


should we bail out early if other == this?

yeah, that branch is part of traditional equals method, let me add

ayushtkn · 2025-09-03T14:29:10Z

+ }
+ }
+
+ public ServiceRecord toServiceRecord() {


this isn't used either?

same as above, adding javadoc

ayushtkn · 2025-09-03T14:33:04Z

+ @Override
+ public int hashCode() {
+ return appId.hashCode() * host.hashCode() * id.hashCode() + port;


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

nice, I haven't used this before :D let me fix

ayushtkn · 2025-09-03T14:36:24Z

 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";


is there a possibility to use TEZ_AM_UUID rather than just UUID?

ayushtkn · 2025-09-03T15:03:32Z

+ Configuration conf = new Configuration();
+ AMRegistry amRegistry = AMRegistryUtils.createAMRegistry(conf);
+ assertNull(amRegistry);
+ String className = "org.apache.tez.dag.api.client.registry.TestAMRegistry$SkeletonAMRegistry";


can we do String className = SkeletonAMRegistry.class.getName(); rather than hardcoding

ayushtkn · 2025-09-03T15:04:15Z

+ amRegistry = AMRegistryUtils.createAMRegistry(conf);
+ assertEquals(className, amRegistry.getClass().getName());


should add
assertNotNull(amRegistry);
Else if it still stays null, rather than failing with some nice error, it will fail with NPE

good catch, adding it

abstractdog · 2025-09-04T10:01:01Z

thanks for the review so far, @ayushtkn, addressed them, precommit is green

tez-yetus · 2025-09-04T10:40:24Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 32s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+0 🆗	xmllint	0m 0s		xmllint was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 3 new or modified test files.
			_ master Compile Tests _
+0 🆗	mvndep	1m 53s		Maven dependency ordering for branch
+1 💚	mvninstall	10m 16s		master passed
+1 💚	compile	1m 24s		master passed
+1 💚	checkstyle	1m 13s		master passed
+1 💚	javadoc	1m 15s		master passed
+0 🆗	spotbugs	2m 18s		tez-api in master has 610 extant spotbugs warnings.
+0 🆗	spotbugs	1m 50s		tez-dag in master has 785 extant spotbugs warnings.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 9s		Maven dependency ordering for patch
+1 💚	mvninstall	0m 59s		the patch passed
+1 💚	codespell	0m 31s		No new issues.
+1 💚	compile	0m 57s		the patch passed
+1 💚	javac	0m 57s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 29s		the patch passed
+1 💚	javadoc	0m 40s		the patch passed
+1 💚	spotbugs	3m 25s		the patch passed
			_ Other Tests _
+1 💚	unit	2m 15s		tez-api in the patch passed.
+1 💚	unit	5m 3s		tez-dag in the patch passed.
+1 💚	asflicense	0m 21s		The patch does not generate ASF License warnings.
		37m 15s

Subsystem	Report/Notes
Docker	ClientAPI=1.51 ServerAPI=1.51 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-426/12/artifact/out/Dockerfile
GITHUB PR	#426
Optional Tests	dupname asflicense codespell detsecrets xmllint javac javadoc unit compile spotbugs checkstyle
uname	Linux 6ceadde259dc 5.15.0-143-generic #153-Ubuntu SMP Fri Jun 13 19:10:45 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	/home/jenkins/jenkins-home/workspace/tez-multibranch_PR-426/src/.yetus/personality.sh
git revision	master / `b05793a`
Default Java	Ubuntu-21.0.8+9-Ubuntu-0ubuntu124.04.1
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-426/12/testReport/
Max. process+thread count	489 (vs. ulimit of 5500)
modules	C: tez-api tez-dag U: .
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-426/12/console
versions	git=2.43.0 maven=3.8.7 spotbugs=4.9.3 codespell=2.0.0
Powered by	Apache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

ayushtkn

LGTM

This comment was marked as outdated.
Sign in to view

TEZ-4008: Pluggable AM FrameworkServices and AmExtensions (2/3)
3fa0220

abstractdog force-pushed the TEZ-4008 branch from ff0acb6 to fa6ec35Compare September 2, 2025 11:09

This comment was marked as outdated.
Sign in to view

abstractdog force-pushed the TEZ-4008 branch 2 times, most recently from 23ce3eb to f0a349cCompare September 2, 2025 14:17

apache deleted a comment from tez-yetus Sep 2, 2025

This comment was marked as outdated.
Sign in to view

abstractdog force-pushed the TEZ-4008 branch 2 times, most recently from 325bcf4 to e08ff20Compare September 2, 2025 17:17

This comment was marked as outdated.
Sign in to view

abstractdog force-pushed the TEZ-4008 branch from e08ff20 to 8febe5bCompare September 2, 2025 18:08

This comment was marked as outdated.
Sign in to view

abstractdog force-pushed the TEZ-4008 branch from 8febe5b to 5a2a6d3Compare September 3, 2025 02:36

This comment was marked as outdated.
Sign in to view

various PR fixes (checkstyle, spotbugs)
6ff2aa5

abstractdog force-pushed the TEZ-4008 branch from 5a2a6d3 to 6ff2aa5Compare September 3, 2025 03:26

abstractdog requested a review from ayushtkn September 3, 2025 03:26

ayushtkn reviewed Sep 3, 2025
View reviewed changes

abstractdog force-pushed the TEZ-4008 branch from 86e29f2 to a0159bdCompare September 4, 2025 08:45

apache deleted a comment from tez-yetus Sep 4, 2025

This comment was marked as outdated.
Sign in to view

review comments
b05793a

abstractdog force-pushed the TEZ-4008 branch from a0159bd to b05793aCompare September 4, 2025 10:02

ayushtkn approved these changes Sep 4, 2025
View reviewed changes

abstractdog merged commit 4a28907 into apache:masterSep 4, 2025
4 checks passed

abstractdog mentioned this pull request Nov 5, 2025
HIVE-29284 Remove tight yarn-registry coupling from hive-exec apache/hive#6146
Closed

		amRegistry = AMRegistryUtils.createAMRegistry(conf);
		assertEquals(className, amRegistry.getClass().getName());

Uh oh!

Conversation

abstractdog commented Sep 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

tez-yetus commented Sep 3, 2025

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abstractdogSep 4, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

abstractdog commented Sep 4, 2025

Uh oh!

tez-yetus commented Sep 4, 2025

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abstractdog commented Sep 2, 2025•
edited
Loading

abstractdogSep 4, 2025•
edited
Loading