TEZ-1037: Replace multiple members in TaskAttemptImpl for container related stuff with a single reference to the container.

[ayushtkn]

No description provided.

[ayushtkn]

I have 3 checkstyle warnings, 2 for unused import, I will fix it. One for creating getter/setter, which looks overkill to me for test purpose and to make checkstyle happy.
Let me know if that is still a required thing, I can add setter/getter

[tez-yetus]

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 39s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	16m 49s	master passed
+1 💚	compile	0m 44s	master passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu122.04.1
+1 💚	compile	0m 38s	master passed with JDK Private Build-1.8.0_362-8u372-gaus1-0ubuntu122.04-b09
+1 💚	checkstyle	1m 20s	master passed
+1 💚	javadoc	0m 33s	master passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu122.04.1
+1 💚	javadoc	0m 21s	master passed with JDK Private Build-1.8.0_362-8u372-gaus1-0ubuntu122.04-b09
+0 🆗	spotbugs	1m 47s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 45s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 29s	the patch passed
+1 💚	compile	0m 33s	the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu122.04.1
+1 💚	javac	0m 33s	the patch passed
+1 💚	compile	0m 26s	the patch passed with JDK Private Build-1.8.0_362-8u372-gaus1-0ubuntu122.04-b09
+1 💚	javac	0m 26s	the patch passed
-0 ⚠️	checkstyle	0m 21s	tez-dag: The patch generated 1 new + 79 unchanged - 1 fixed = 80 total (was 80)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 9s	the patch passed with JDK Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu122.04.1
+1 💚	javadoc	0m 9s	the patch passed with JDK Private Build-1.8.0_362-8u372-gaus1-0ubuntu122.04-b09
+1 💚	findbugs	1m 16s	the patch passed
		_ Other Tests _
+1 💚	unit	5m 6s	tez-dag in the patch passed.
+1 💚	asflicense	0m 12s	The patch does not generate ASF License warnings.
		32m 37s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/3/artifact/out/Dockerfile
GITHUB PR	#305
JIRA Issue	TEZ-1037
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux 268d13160038 4.15.0-212-generic #223-Ubuntu SMP Tue May 23 13:09:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / 5beab4c
Default Java	Private Build-1.8.0_362-8u372-gaus1-0ubuntu122.04-b09
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu122.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u372-gaus1-0ubuntu122.04-b09
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/3/artifact/out/diff-checkstyle-tez-dag.txt
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/3/testReport/
Max. process+thread count	214 (vs. ulimit of 5500)
modules	C: tez-dag U: tez-dag
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/3/console
versions	git=2.34.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

[abstractdog]

I can see that we're doing lots of interning stuff in the TaskAttemptImpl, which kind of looks like boilerplate from the TaskAttempImpl class' point of view
however, I believe these intern calls were done for a reason: what about wrapping this yarn container into a tez implementation that can hide all the intern + null checks + whatever? so
instead of:

StringInterner.intern(RackResolver.resolve(container.getNodeId().getHost()).getNetworkLocation());

let it be:

container.getNodeRackName();

basically, a new TezContainer (or name it as you like) class can have all these fields that you're about to remove from TaskAttemptImpl

optionally: look for usages of org.apache.hadoop.yarn.api.records.Container in tez code and remove it from there too (it can be done in followup tickets)

[ayushtkn]

@abstractdog I have addressed the review comments, added a new TezContainer Class & pushed the methods there, let me know if it looks good.

One checkstyle is left intentionally, that visibility is changed for test & satisfying that looked like an overkill

[abstractdog]

please add a class javadoc comment clarifying the purpose of this class
which is basically to have a convenience wrapper around org.apache.hadoop.yarn.api.records.Container

[abstractdog]

@abstractdog I have addressed the review comments, added a new TezContainer Class & pushed the methods there, let me know if it looks good.

One checkstyle is left intentionally, that visibility is changed for test & satisfying that looked like an overkill

everything looks good, left a minor comment about TezContainer, otherwise +1

[tez-yetus]

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 33s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 1 new or modified test files.
		_ master Compile Tests _
+1 💚	mvninstall	13m 18s	master passed
+1 💚	compile	0m 44s	master passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu122.04
+1 💚	compile	0m 37s	master passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚	checkstyle	1m 21s	master passed
+1 💚	javadoc	0m 32s	master passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu122.04
+1 💚	javadoc	0m 20s	master passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+0 🆗	spotbugs	1m 47s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 45s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 27s	the patch passed
+1 💚	compile	0m 31s	the patch passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu122.04
+1 💚	javac	0m 31s	the patch passed
+1 💚	compile	0m 26s	the patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚	javac	0m 26s	the patch passed
-0 ⚠️	checkstyle	0m 21s	tez-dag: The patch generated 2 new + 79 unchanged - 1 fixed = 81 total (was 80)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 9s	the patch passed with JDK Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu122.04
+1 💚	javadoc	0m 8s	the patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚	findbugs	1m 15s	the patch passed
		_ Other Tests _
+1 💚	unit	5m 0s	tez-dag in the patch passed.
+1 💚	asflicense	0m 14s	The patch does not generate ASF License warnings.
		28m 48s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/9/artifact/out/Dockerfile
GITHUB PR	#305
JIRA Issue	TEZ-1037
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux 77ba9496ad48 4.15.0-212-generic #223-Ubuntu SMP Tue May 23 13:09:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / 5beab4c
Default Java	Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.20+8-post-Ubuntu-1ubuntu122.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/9/artifact/out/diff-checkstyle-tez-dag.txt
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/9/testReport/
Max. process+thread count	217 (vs. ulimit of 5500)
modules	C: tez-dag U: tez-dag
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-305/9/console
versions	git=2.34.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.