Skip to content

TEZ-4365: Use Regex Pattern to Parse DAG ID String - #172

Open
belugabehr wants to merge 2 commits into
apache:masterfrom
belugabehr:TEZ-4365
Open

TEZ-4365: Use Regex Pattern to Parse DAG ID String#172
belugabehr wants to merge 2 commits into
apache:masterfrom
belugabehr:TEZ-4365

Conversation

@belugabehr

Copy link
Copy Markdown
Contributor

No description provided.

@tez-yetus

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec16m 27sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 1 new or modified test files.
_ master Compile Tests _
+1 💚mvninstall12m 59smaster passed
+1 💚compile0m 23smaster passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚compile0m 22smaster passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚checkstyle0m 53smaster passed
+1 💚javadoc0m 33smaster passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚javadoc0m 19smaster passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗spotbugs1m 0sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs0m 57smaster passed
_ Patch Compile Tests _
+1 💚mvninstall0m 13sthe patch passed
+1 💚compile0m 13sthe patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚javac0m 13sthe patch passed
+1 💚compile0m 11sthe patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚javac0m 11sthe patch passed
+1 💚checkstyle0m 8sthe patch passed
-1 ❌whitespace0m 0sThe patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1 💚javadoc0m 12sthe patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚javadoc0m 11sthe patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚findbugs0m 32sthe patch passed
_ Other Tests _
+1 💚unit0m 30stez-common in the patch passed.
+1 💚asflicense0m 13sThe patch does not generate ASF License warnings.
36m 4s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-172/1/artifact/out/Dockerfile
GITHUB PR#172
JIRA IssueTEZ-4365
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
unameLinux bb1b434b01c7 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitypersonality/tez.sh
git revisionmaster / c9b8e90
Default JavaPrivate 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
whitespacehttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-172/1/artifact/out/whitespace-eol.txt
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-172/1/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-172/1/console
versionsgit=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@tez-yetus

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 37sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 1 new or modified test files.
_ master Compile Tests _
+1 💚mvninstall14m 42smaster passed
+1 💚compile0m 27smaster passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚compile0m 23smaster passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚checkstyle0m 59smaster passed
+1 💚javadoc0m 35smaster passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚javadoc0m 24smaster passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗spotbugs1m 1sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs0m 59smaster passed
_ Patch Compile Tests _
+1 💚mvninstall0m 15sthe patch passed
+1 💚compile0m 15sthe patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚javac0m 15sthe patch passed
+1 💚compile0m 14sthe patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚javac0m 14sthe 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.13+8-Ubuntu-0ubuntu1.20.04
+1 💚javadoc0m 13sthe patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚findbugs0m 38sthe patch passed
_ Other Tests _
+1 💚unit0m 32stez-common in the patch passed.
+1 💚asflicense0m 14sThe patch does not generate ASF License warnings.
23m 44s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-172/2/artifact/out/Dockerfile
GITHUB PR#172
JIRA IssueTEZ-4365
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
unameLinux 8454a0c9589f 4.15.0-163-generic #171-Ubuntu SMP Fri Nov 5 11:55:11 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitypersonality/tez.sh
git revisionmaster / c9b8e90
Default JavaPrivate 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
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-172/2/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-172/2/console
versionsgit=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@jteagles

Copy link
Copy Markdown
Contributor

@belugabehr, can you go back and run the performance tests in https://issues.apache.org/jira/browse/TEZ-1526. It will be interesting to see how this performs after removing the performance optimizations.

@belugabehr

Copy link
Copy Markdown
ContributorAuthor

@jteagles I create a small driver with JMH:

Benchmark Mode Cnt Score Error Units
TestSplit.master thrpt 30 5324642.492 ± 228078.761 ops/s
TestSplit.tez4365 thrpt 30 1809324.533 ± 37792.272 ops/s

Quite a bit slower, but still an impressive 1,809,324 string per second on my dated hardware. Using regex provides for fewer lines of code and makes it more readable. But your call. If you're not accepting of it, consider the unit tests update.

@jteagles

Copy link
Copy Markdown
Contributor

This code optimization was critically import as the the event thread spends a significant time parsing task/attempt ids to dispatch messages. I would hate to lose that. I can appreciate the simplicity of REGEX though. Perhaps the regex can be used to validate the manual parsing, as the manual parsing is more error prone. And improved testing is welcome.

This patch inspired a YARN ID parsing improvement that made significant improvements there as well. I've linked the jira for reference in the original. https://issues.apache.org/jira/browse/YARN-6768

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.

3 participants

@belugabehr@tez-yetus@jteagles