Skip to content

TEZ-4487: Add class name profiling option in ProfileServlet - #281

Merged
abstractdog merged 2 commits into
apache:masterfrom
difin:TEZ-4487
Apr 19, 2023
Merged

TEZ-4487: Add class name profiling option in ProfileServlet#281
abstractdog merged 2 commits into
apache:masterfrom
difin:TEZ-4487

Conversation

@difin

@difindifin commented Apr 10, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?
Adding support for method name profiling to Tez's ProfileServlet.

Why are the changes needed?
Tez has ProfileServlet. It makes use of async-profiler and allows profiling specific events. Currently profileServlet supports events like cpu, alloc, lock etc. It will be good to enhance to support method name profiling as well.

Does this PR introduce any user-facing change?
No

How was this patch tested?
Automated pre-commit tests;
Manual testing was done as part of HIVE-27184 and the same changes are proposed here.

@tez-yetus

This comment was marked as outdated.

@abstractdog
abstractdog self-requested a review April 13, 2023 08:51

@abstractdogabstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor formatting issue, other than that LGTM

Comment threadtez-common/src/main/java/org/apache/tez/common/web/ProfileServlet.java Outdated
@tez-yetus

This comment was marked as outdated.

@difin

Copy link
Copy Markdown
ContributorAuthor

Hi @abstractdog , I fixed the line that had a whitespace, but it says there is one more line that ends with a whitespace. I checked all the lines that I changed and couldn't find additional whitespace.

@abstractdog
abstractdog self-requested a review April 14, 2023 07:01
@abstractdog

abstractdog commented Apr 14, 2023

Copy link
Copy Markdown
Contributor

Hi @abstractdog , I fixed the line that had a whitespace, but it says there is one more line that ends with a whitespace. I checked all the lines that I changed and couldn't find additional whitespace.

@difin: there is indeed a whitespace in line 208, checked locally

+ response.getWriter().write("Event and method aren't allowed to be both used in the same request.");
+ return;
+ }
+ <-- here
if (process == null || !process.isAlive()) {
try {

@aturoczy

Copy link
Copy Markdown

@difin

Copy link
Copy Markdown
ContributorAuthor

Hi @abstractdog,
Thanks, removed the trailing whitespace.
Can you please review again?

@tez-yetus

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec24m 57sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚@author0m 0sThe patch does not contain any @author tags.
-1 ❌test4tests0m 0sThe 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 💚mvninstall15m 37smaster passed
+1 💚compile0m 23smaster passed with JDK Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu122.04
+1 💚compile0m 21smaster passed with JDK Private Build-1.8.0_362-8u362-ga-0ubuntu1~22.04-b09
+1 💚checkstyle0m 53smaster passed
+1 💚javadoc0m 31smaster passed with JDK Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu122.04
+1 💚javadoc0m 20smaster passed with JDK Private Build-1.8.0_362-8u362-ga-0ubuntu1~22.04-b09
+0 🆗spotbugs0m 57sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs0m 55smaster passed
_ Patch Compile Tests _
+1 💚mvninstall0m 13sthe patch passed
+1 💚compile0m 13sthe patch passed with JDK Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu122.04
+1 💚javac0m 13sthe patch passed
+1 💚compile0m 12sthe patch passed with JDK Private Build-1.8.0_362-8u362-ga-0ubuntu1~22.04-b09
+1 💚javac0m 12sthe patch passed
+1 💚checkstyle0m 8sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚javadoc0m 13sthe patch passed with JDK Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu122.04
+1 💚javadoc0m 11sthe patch passed with JDK Private Build-1.8.0_362-8u362-ga-0ubuntu1~22.04-b09
+1 💚findbugs0m 35sthe patch passed
_ Other Tests _
+1 💚unit0m 29stez-common in the patch passed.
+1 💚asflicense0m 11sThe patch does not generate ASF License warnings.
47m 15s
SubsystemReport/Notes
DockerClientAPI=1.42 ServerAPI=1.42 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-281/3/artifact/out/Dockerfile
GITHUB PR#281
JIRA IssueTEZ-4487
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
unameLinux b97efeff0012 4.15.0-206-generic #217-Ubuntu SMP Fri Feb 3 19:10:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitypersonality/tez.sh
git revisionmaster / 9a729cd
Default JavaPrivate Build-1.8.0_362-8u362-ga-0ubuntu1~22.04-b09
Multi-JDK versions/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu122.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_362-8u362-ga-0ubuntu1~22.04-b09
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-281/3/testReport/
Max. process+thread count91 (vs. ulimit of 5500)
modulesC: tez-common U: tez-common
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-281/3/console
versionsgit=2.34.1 maven=3.6.3 findbugs=3.0.1
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@abstractdog
abstractdog merged commit 2fe3c46 into apache:masterApr 19, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@difin@tez-yetus@abstractdog@aturoczy