TEZ-4350: Remove synchronized from DAGAppMaster.serviceInit

[abstractdog]

No description provided.

[tez-yetus]

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	16m 56s	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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ master Compile Tests _
+1 💚	mvninstall	13m 32s	master passed
+1 💚	compile	0m 47s	master passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	0m 39s	master passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	checkstyle	1m 6s	master passed
+1 💚	javadoc	0m 48s	master passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 34s	master passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+0 🆗	spotbugs	1m 51s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 50s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 30s	the patch passed
+1 💚	compile	0m 36s	the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	0m 36s	the patch passed
+1 💚	compile	0m 29s	the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	javac	0m 29s	the patch passed
+1 💚	checkstyle	0m 22s	the patch passed
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 27s	the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 23s	the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
-1 ❌	findbugs	1m 29s	tez-dag generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0)
		_ Other Tests _
+1 💚	unit	4m 34s	tez-dag in the patch passed.
+1 💚	asflicense	0m 13s	The patch does not generate ASF License warnings.
		46m 7s

Reason	Tests
FindBugs	module:tez-dag
	Inconsistent synchronization of org.apache.tez.dag.app.DAGAppMaster.dagEventDispatcher; locked 66% of time Unsynchronized access at DAGAppMaster.java:66% of time Unsynchronized access at DAGAppMaster.java:[line 540]
	Inconsistent synchronization of org.apache.tez.dag.app.DAGAppMaster.sessionTimeoutInterval; locked 83% of time Unsynchronized access at DAGAppMaster.java:83% of time Unsynchronized access at DAGAppMaster.java:[line 593]

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/1/artifact/out/Dockerfile
GITHUB PR	#162
JIRA Issue	TEZ-4350
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux cc83338c7e69 4.15.0-143-generic #147-Ubuntu SMP Wed Apr 14 16:10:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / f39a51e
Default Java	Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
findbugs	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/1/artifact/out/new-findbugs-tez-dag.html
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/1/testReport/
Max. process+thread count	178 (vs. ulimit of 5500)
modules	C: tez-dag U: tez-dag
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/1/console
versions	git=2.25.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.

[jteagles]

@abstractdog, the findbugs above is showing the inconsistent synchronization. I am trying to imagine scenarios where the synchronization is needed, but let me describe generally what could happen. In multi-threaded apps, assignments are lazy and in a busy time, java may choose not to immediately propagate value change or assignment across to all threads. Then Other threads accessing will find an outdated value or old or unassigned object. So it's always safer to synchronize all accesses. However, it's rare to have an "init" or initial object assignment not get propagated to all other threads. My knowledge about this might be a little bit incorrect, so correct me if I'm wrong. The question to me is really, what is the cost of having the synchronization there? My guess would be a millisecond max, which in the grand scheme of things, isn't much. Do have think this change will have a bigger impact?

[abstractdog]

let me give the background of this change: it was created exactly because of the same inconsistent sync alerts in TEZ-4347 (which I'm about to finish soon too), just because in TEZ-4347 I was accessing some fields, that I don't really worried about from synchronization point of view, e.g. webUIService (alert is here), but it's accessed from serviceInit too, which doesn't need to be synchronized according to comments

this means two ways to solve this:
[1] make every access to every variable synchronized which is accessed from serviceInit (only because serviceInit is synchronized at the moment)
[2] consider removing synchronized from serviceInit if possible

[1] doesn't make sense to me, as serviceInit itself is not supposed to be synchronized
[2] I want to go this way, even if it gave an inconsistent sync alert (that needs to be checked), but in the opposite way as I saw in TEZ-4347, so here I need to revise if every field accessed from serviceInit should be really accessed synchronized every time

so my point is, the goal here is not performance, but going for [2] instead of [1]

[abstractdog]

double-checked: AbstractService lifecycle methods are guarded with a lock since YARN-530 (which introduced a strict and robust lifecycle)
that's why serviceInit, serviceStart, serviceStop is not supposed to be guarded in AbstractService subclasses
I also found that this change has let me remove lots of inconsistent sync warnings from findbugs-exclude.xml config (basically: these 3 methods touch a lot of DAGAppMaster fields, by which findbugs thinks that they needs synchronized access during the whole DAGAppMaster lifecycle)

YARN-530 + MAPREDUCE-5298 introduced similar changes to MrAppMaster

[tez-yetus]

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	12m 45s	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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
		_ master Compile Tests _
+1 💚	mvninstall	13m 56s	master passed
+1 💚	compile	0m 42s	master passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚	compile	0m 38s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 11s	master passed
+1 💚	javadoc	0m 48s	master passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 37s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗	spotbugs	1m 40s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	1m 38s	master passed
		_ Patch Compile Tests _
+1 💚	mvninstall	0m 25s	the patch passed
+1 💚	compile	0m 27s	the patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚	javac	0m 27s	the patch passed
+1 💚	compile	0m 25s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	0m 25s	the patch passed
-0 ⚠️	checkstyle	0m 19s	tez-dag: The patch generated 1 new + 52 unchanged - 1 fixed = 53 total (was 53)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	xml	0m 1s	The patch has no ill-formed XML file.
+1 💚	javadoc	0m 22s	the patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚	javadoc	0m 21s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	findbugs	1m 10s	the patch passed
		_ Other Tests _
+1 💚	unit	4m 29s	tez-dag in the patch passed.
+1 💚	asflicense	0m 15s	The patch does not generate ASF License warnings.
		41m 22s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/3/artifact/out/Dockerfile
GITHUB PR	#162
JIRA Issue	TEZ-4350
Optional Tests	dupname asflicense xml javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux 0f275da5d0a7 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / 267ca11
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/3/artifact/out/diff-checkstyle-tez-dag.txt
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/3/testReport/
Max. process+thread count	188 (vs. ulimit of 5500)
modules	C: tez-dag U: tez-dag
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-162/3/console
versions	git=2.25.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.

[jteagles]

+1. Reran Tez QA bot to verify findbugs results.