Skip to content

TEZ-4521: Partition stats should be always uncompressed size - #317

Merged
abstractdog merged 1 commit into
apache:masterfrom
okumin:TEZ-4521-uncompressed
Nov 28, 2023
Merged

TEZ-4521: Partition stats should be always uncompressed size#317
abstractdog merged 1 commit into
apache:masterfrom
okumin:TEZ-4521-uncompressed

Conversation

@okumin

Copy link
Copy Markdown
Contributor

Currently, we need to configure the compressed size for ordered outputs and the decompressed size for unordered output. It makes sense if we can have consistent semantics.
https://issues.apache.org/jira/browse/TEZ-4521

@okumin
okumin marked this pull request as ready for review November 27, 2023 15:31
@okuminokumin changed the title [WIP] TEZ-4521: Partition stats should be always uncompressed sizeTEZ-4521: Partition stats should be always uncompressed sizeNov 27, 2023
@tez-yetus

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec13m 19sDocker 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 💚mvninstall15m 15smaster passed
+1 💚compile0m 22smaster passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚compile0m 19smaster passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚checkstyle1m 12smaster passed
+1 💚javadoc0m 32smaster passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javadoc0m 19smaster passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+0 🆗spotbugs1m 7sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs1m 6smaster passed
_ Patch Compile Tests _
+1 💚mvninstall0m 13sthe patch passed
+1 💚compile0m 13sthe patch passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javac0m 13sthe patch passed
+1 💚compile0m 12sthe patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚javac0m 12sthe patch passed
+1 💚checkstyle0m 11sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚javadoc0m 12sthe patch passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javadoc0m 11sthe patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚findbugs0m 35sthe patch passed
_ Other Tests _
+1 💚unit4m 23stez-runtime-library in the patch passed.
+1 💚asflicense0m 15sThe patch does not generate ASF License warnings.
39m 25s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-317/1/artifact/out/Dockerfile
GITHUB PR#317
JIRA IssueTEZ-4521
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
unameLinux 45f7623bdf4c 5.15.0-88-generic #98-Ubuntu SMP Mon Oct 2 15:18:56 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitypersonality/tez.sh
git revisionmaster / 51d6f53
Default JavaPrivate 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.1+1-post-Ubuntu-0ubuntu122.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-317/1/testReport/
Max. process+thread count2100 (vs. ulimit of 5500)
modulesC: tez-runtime-library U: tez-runtime-library
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-317/1/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.

// Use partition sizes to compute the total size.
if (partitionSizes != null) {
totalSize = estimatedUncompressedSum(partitionSizes);
totalSize = Arrays.stream(partitionSizes).sum();

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.

does totalSize change with this patch? if it doesn't, why? if it does, can we validate it with this unit test or in anyway that makes sense to you @okumin ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This patch doesn't change the total size. That's because the total size is stored in a different field of Protbuf from partitions stats. The design is valid since users have an option not to take partition stats at all(tez.runtime.report.partition.stats=none).
TEZ-4521 would remove the possibility where partition stats contain the compressed size. That's why I revised this file to prevent future users from being confused.

@abstractdog

Copy link
Copy Markdown
Contributor

thanks @okumin for this patch, I've put a minor comment

@okumin

Copy link
Copy Markdown
ContributorAuthor

@abstractdog Thanks! This patch is related to #306 and I'd be glad if you could take a look at it.

@abstractdog
abstractdog self-requested a review November 28, 2023 15:30
@abstractdog
abstractdog merged commit 43562ad into apache:masterNov 28, 2023
@abstractdog

Copy link
Copy Markdown
Contributor

merged, thanks @okumin
I'll check the rest of this fair scheduling work as soon as I can, I promise :)

@okumin
okumin deleted the TEZ-4521-uncompressed branch November 28, 2023 15:35
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

@okumin@tez-yetus@abstractdog