Skip to content

TEZ-4503: Warn about large conf properties in payload - #308

Open
zabetak wants to merge 5 commits into
apache:masterfrom
zabetak:TEZ-4503
Open

TEZ-4503: Warn about large conf properties in payload#308
zabetak wants to merge 5 commits into
apache:masterfrom
zabetak:TEZ-4503

Conversation

@zabetak

@zabetakzabetak commented Sep 14, 2023

Copy link
Copy Markdown
Member

The changes were tested by running mvn test -Dtest=TestTezUtils and manually inspecting the output under: tez-common/target/surefire-reports/org.apache.tez.common.TestTezUtils-output.txt .

SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:/home/stamatis/.m2/repository/org/slf4j/slf4j-reload4j/1.7.36/slf4j-reload4j-1.7.36.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:/home/stamatis/.m2/repository/org/slf4j/slf4j-log4j12/1.7.25/slf4j-log4j12-1.7.25.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Reload4jLoggerFactory]
2023-09-14 15:03:08,732 WARN [Time-limited test] common.TezUtils (TezUtils.java:logEntryIfLarge(237)) - Property 'testLargeValue' is unusually big (67108886 bytes); large payload may lead to OOM.
2023-09-14 15:03:08,841 INFO [Time-limited test] common.TezUtils (TezUtils.java:logEntryIfLarge(239)) - Large property 'testLargeValue': ABCDEFGHIJKLMNOPQRSTUVWXYZABCDEFGHIJKLMNOPQRSTUVWXYZABCDEFGHIJKLMNOPQRSTUVWXYZABCDE...
Additionally, I added a `tez-site.xml` file under `tez-common/test/resources` and varied a bit the values for the new configs.

@tez-yetus

This comment was marked as outdated.

Sign in to view
@tez-yetus

tez-yetus commented Sep 14, 2023

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 18sDocker 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 💚mvninstall9m 57smaster passed
+1 💚compile0m 35smaster passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚compile0m 35smaster passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚checkstyle0m 37smaster passed
+1 💚javadoc0m 45smaster passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javadoc0m 42smaster passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+0 🆗spotbugs1m 49sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs1m 47smaster passed
_ Patch Compile Tests _
+1 💚mvninstall0m 24sthe patch passed
+1 💚compile0m 27sthe patch passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javac0m 27sthe patch passed
+1 💚compile0m 23sthe patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚javac0m 23sthe patch passed
+1 💚checkstyle0m 14sthe 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 26sthe patch passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javadoc0m 26sthe patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚findbugs1m 9sthe patch passed
_ Other Tests _
+1 💚unit2m 17stez-api in the patch passed.
+1 💚asflicense0m 16sThe patch does not generate ASF License warnings.
22m 26s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/2/artifact/out/Dockerfile
GITHUB PR#308
JIRA IssueTEZ-4503
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
unameLinux 9309314f74f7 4.15.0-212-generic #223-Ubuntu SMP Tue May 23 13:09:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitypersonality/tez.sh
git revisionmaster / 7855c1f
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
whitespacehttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/2/artifact/out/whitespace-eol.txt
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/2/testReport/
Max. process+thread count571 (vs. ulimit of 5500)
modulesC: tez-api U: tez-api
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/2/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.

@tez-yetus

This comment was marked as outdated.

Sign in to view
@tez-yetus

This comment was marked as outdated.

Sign in to view
@zabetak

zabetak commented Sep 18, 2023

Copy link
Copy Markdown
MemberAuthor

@abstractdog Can you please have a look in this PR when you get the chance. Thanks!

abstractdog
abstractdog reviewed Oct 17, 2023
View reviewed changes
Comment threadtez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java
public static final String TEZ_LOGGING_PROPERTY_SIZE_THRESHOLD =
TEZ_PREFIX + "logging.property.size.threshold";
public static final int TEZ_LOGGING_PROPERTY_SIZE_THRESHOLD_DEFAULT = 512 * 1024;
/**

@abstractdogabstractdogOct 17, 2023

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.

nit, we tend to add a line between every config properties, even if they are related to each other

@zabetakzabetakOct 27, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fixed in d8215af

abstractdog
abstractdog reviewed Oct 17, 2023
View reviewed changes
Comment threadtez-api/src/main/java/org/apache/tez/common/TezUtils.java Outdated
private static final boolean PROPERTY_MASK;

static {
TezConfiguration c = new TezConfiguration();

@abstractdogabstractdogOct 17, 2023

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.

I'm afraid this approach will become confusing hard to handle over time
a new TezConfiguration() in a static initializer will always pick up whatever is present in tez-site.xml, hence we don't really have control over that: I think if we want to be able to use the size threshold, we need to achieve that from the actual config, which might need a bit of refactoring as this method receives Configuration as an iterable (and we need to see the threshold and mask property beforehand without iterating over the values)

@zabetakzabetakOct 27, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I reworked the PR (https://github.com/apache/tez/pull/308/commits/17e07e7c714d670cfbd5e15eb3c56f33a022f2ec,https://github.com/apache/tez/pull/308/commits/687bd3b22f26288c0f49d40d05588d4f17a7b2fa) to allow using the size threshold from within the configuration object.

@zabetak
Allow different logging threshold per configuration object
17e07e7
Instead of using the central Tez configuration (tez-site.xml) for selecting the threshold and masking for logging large properties check for the respective entries in each individual configuration object.
@zabetak
Refactor to use native JDK APIs instead of Guava
687bd3b
@zabetak
Add newline between new properties
d8215af
@zabetak
Add tests for new method and to check logs content
1253d18
@tez-yetus

tez-yetus commented Oct 27, 2023

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec24m 52sDocker 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 2 new or modified test files.
_ master Compile Tests _
+0 🆗mvndep12m 46sMaven dependency ordering for branch
+1 💚mvninstall15m 43smaster passed
+1 💚compile3m 38smaster passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚compile3m 19smaster passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚checkstyle2m 38smaster passed
+1 💚javadoc2m 44smaster passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javadoc2m 21smaster passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+0 🆗spotbugs5m 47sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs8m 22smaster passed
-0 ⚠️patch6m 8sUsed diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
_ Patch Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for patch
+1 💚mvninstall4m 46sthe patch passed
+1 💚compile3m 36sthe patch passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javac3m 36sthe patch passed
+1 💚compile3m 21sthe patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚javac3m 21sthe patch passed
-0 ⚠️checkstyle0m 31stez-api: The patch generated 1 new + 72 unchanged - 1 fixed = 73 total (was 73)
+1 💚checkstyle0m 26stez-common: The patch generated 0 new + 14 unchanged - 1 fixed = 14 total (was 15)
-0 ⚠️checkstyle0m 57sroot: The patch generated 1 new + 86 unchanged - 2 fixed = 87 total (was 88)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚xml0m 1sThe patch has no ill-formed XML file.
+1 💚javadoc2m 22sthe patch passed with JDK Ubuntu-11.0.20.1+1-post-Ubuntu-0ubuntu122.04
+1 💚javadoc2m 17sthe patch passed with JDK Private Build-1.8.0_382-8u382-ga-1~22.04.1-b05
+1 💚findbugs7m 54sthe patch passed
_ Other Tests _
+1 💚unit2m 36stez-api in the patch passed.
+1 💚unit0m 44stez-common in the patch passed.
+1 💚unit75m 59sroot in the patch passed.
+1 💚asflicense1m 27sThe patch does not generate ASF License warnings.
186m 40s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/5/artifact/out/Dockerfile
GITHUB PR#308
JIRA IssueTEZ-4503
Optional Testsdupname asflicense javac javadoc unit xml compile spotbugs findbugs checkstyle
unameLinux ff9dd343b5c7 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 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
checkstylehttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/5/artifact/out/diff-checkstyle-tez-api.txt
checkstylehttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/5/artifact/out/diff-checkstyle-root.txt
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/5/testReport/
Max. process+thread count1589 (vs. ulimit of 5500)
modulesC: tez-api tez-common . U: .
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-308/5/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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abstractdogabstractdogabstractdog left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zabetak@tez-yetus@abstractdog