YARN-10909. AbstractCSQueue: Annotate all methods with VisibleForTesting that are only used by test code #3360

JackWangCS · 2021-08-31T15:09:39Z

Description of PR

AbstractCSQueue: some methods added for test code but not annotated with VisibleForTesting.

How was this patch tested?

Just code style fix, no new tests needed

For code changes:

[Yes] Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
[No] Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
[No] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
[No] If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

hadoop-yetus · 2021-08-31T18:15:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	12m 37s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+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.
			_ trunk Compile Tests _
+1 💚	mvninstall	31m 16s		trunk passed
+1 💚	compile	1m 3s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	0m 56s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	checkstyle	0m 49s		trunk passed
+1 💚	mvnsite	1m 1s		trunk passed
+1 💚	javadoc	0m 48s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 43s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 51s		trunk passed
+1 💚	shadedclient	15m 10s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 50s		the patch passed
+1 💚	compile	0m 53s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	0m 53s		the patch passed
+1 💚	compile	0m 46s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	javac	0m 46s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 38s		the patch passed
+1 💚	mvnsite	0m 50s		the patch passed
+1 💚	javadoc	0m 38s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 35s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 51s		the patch passed
+1 💚	shadedclient	14m 57s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	95m 38s		hadoop-yarn-server-resourcemanager in the patch passed.
+1 💚	asflicense	0m 35s		The patch does not generate ASF License warnings.
		184m 10s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3360/1/artifact/out/Dockerfile
GITHUB PR	#3360
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell
uname	Linux 5e286f0ad2ac 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	dev-support/bin/hadoop.sh
git revision	trunk / `1e5af11`
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
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3360/1/testReport/
Max. process+thread count	939 (vs. ulimit of 5500)
modules	C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3360/1/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

brumi1024

@JackWangCS thanks for working on this. Actually there are a few more cases where the only reason the method is not private is because it is used in tests (for example recoverDrainingState), can you please address those as well?

JackWangCS · 2021-09-08T15:39:15Z

Hi @brumi1024, sorry, I am a bit confused. I don't find recoverDrainingState() is called in tests. One call site is at CapacityScheduler#addApplicationOnRecovery() when CapacityScheduler tries to handle the Application recovery.

brumi1024 · 2021-09-08T16:12:34Z

@JackWangCS, my mistake, sorry, mixed it up with another method. I have one small nit: getLastSubmittedTimestamp and setLastSubmittedTimestamp is now package private, but updateLastSubmittedTimeStamp is public and they operate on the same variable. Can you please change it as well? Otherwise +1 from my side.

JackWangCS · 2021-09-09T02:05:40Z

@brumi1024 I have changed the setLastSubmittedTimestamp to package private. Thanks for your review.

hadoop-yetus · 2021-09-09T05:09:05Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 53s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+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.
			_ trunk Compile Tests _
+1 💚	mvninstall	33m 37s		trunk passed
+1 💚	compile	1m 1s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	0m 52s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	checkstyle	0m 46s		trunk passed
+1 💚	mvnsite	0m 59s		trunk passed
+1 💚	javadoc	0m 55s		trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 39s		trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 57s		trunk passed
+1 💚	shadedclient	17m 57s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 51s		the patch passed
+1 💚	compile	0m 57s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	0m 57s		the patch passed
+1 💚	compile	0m 46s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	javac	0m 46s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 38s		the patch passed
+1 💚	mvnsite	0m 50s		the patch passed
+1 💚	javadoc	0m 39s		the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	0m 34s		the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚	spotbugs	1m 59s		the patch passed
+1 💚	shadedclient	17m 20s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	99m 35s		hadoop-yarn-server-resourcemanager in the patch passed.
+1 💚	asflicense	0m 29s		The patch does not generate ASF License warnings.
		183m 21s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3360/2/artifact/out/Dockerfile
GITHUB PR	#3360
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell
uname	Linux c97a2869c059 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `b2506d7`
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
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3360/2/testReport/
Max. process+thread count	907 (vs. ulimit of 5500)
modules	C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3360/2/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java

szilard-nemeth · 2021-10-23T12:46:17Z

Hi @JackWangCS,
Thanks for working on this.
I checked the method usages for all methods and didn't find any method in AbstractCSQueue that would be accessed only by test code, so I can't add anything more to this patch.
Latest patch LGTM, committed to trunk.

add @VisibleForTesting for only testing used code

1e5af11

brumi1024 requested changes Sep 8, 2021

View reviewed changes

change updateLastSubmittedTimeStamp to package private

b2506d7

brumi1024 approved these changes Sep 9, 2021

View reviewed changes

szilard-nemeth requested changes Sep 10, 2021

View reviewed changes

...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java Show resolved Hide resolved

...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java Show resolved Hide resolved

szilard-nemeth approved these changes Sep 10, 2021

View reviewed changes

...n/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java Show resolved Hide resolved

szilard-nemeth self-assigned this Oct 23, 2021

szilard-nemeth changed the title ~~YARN-10909. AbstractCSQueue: add @VisibleForTesting annotation for test code~~ YARN-10909. AbstractCSQueue: Annotate all methods with VisibleForTesting that are only used by test code Oct 23, 2021

szilard-nemeth assigned JackWangCS Oct 23, 2021

szilard-nemeth closed this Oct 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

YARN-10909. AbstractCSQueue: Annotate all methods with VisibleForTesting that are only used by test code #3360

YARN-10909. AbstractCSQueue: Annotate all methods with VisibleForTesting that are only used by test code #3360

JackWangCS commented Aug 31, 2021

hadoop-yetus commented Aug 31, 2021

brumi1024 left a comment

JackWangCS commented Sep 8, 2021

brumi1024 commented Sep 8, 2021

JackWangCS commented Sep 9, 2021

hadoop-yetus commented Sep 9, 2021

szilard-nemeth commented Oct 23, 2021

YARN-10909. AbstractCSQueue: Annotate all methods with VisibleForTesting that are only used by test code #3360

YARN-10909. AbstractCSQueue: Annotate all methods with VisibleForTesting that are only used by test code #3360

Conversation

JackWangCS commented Aug 31, 2021

Description of PR

How was this patch tested?

For code changes:

hadoop-yetus commented Aug 31, 2021

brumi1024 left a comment

Choose a reason for hiding this comment

JackWangCS commented Sep 8, 2021

brumi1024 commented Sep 8, 2021

JackWangCS commented Sep 9, 2021

hadoop-yetus commented Sep 9, 2021

szilard-nemeth commented Oct 23, 2021