Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0be881d
TEZ-4503: Warn about large conf properties in payload
zabetak Sep 14, 2023
17e07e7
Allow different logging threshold per configuration object
zabetak Oct 23, 2023
687bd3b
Refactor to use native JDK APIs instead of Guava
zabetak Oct 24, 2023
d8215af
Add newline between new properties
zabetak Oct 24, 2023
1253d18
Add tests for new method and to check logs content
zabetak Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions tez-api/src/main/java/org/apache/tez/common/TezUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.protobuf.ByteString;

import com.google.protobuf.CodedInputStream;
import org.apache.tez.dag.api.TezConfiguration;
import org.apache.tez.runtime.api.TaskContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -43,6 +44,11 @@
import org.xerial.snappy.SnappyInputStream;
import org.xerial.snappy.SnappyOutputStream;

import static org.apache.tez.dag.api.TezConfiguration.TEZ_LOGGING_PROPERTY_MASK;
import static org.apache.tez.dag.api.TezConfiguration.TEZ_LOGGING_PROPERTY_MASK_DEFAULT;
import static org.apache.tez.dag.api.TezConfiguration.TEZ_LOGGING_PROPERTY_SIZE_THRESHOLD;
import static org.apache.tez.dag.api.TezConfiguration.TEZ_LOGGING_PROPERTY_SIZE_THRESHOLD_DEFAULT;

/**
* Utility methods for setting up a DAG. Has helpers for setting up log4j configuration, converting
* {@link org.apache.hadoop.conf.Configuration} to {@link org.apache.tez.dag.api.UserPayload} etc.
Expand All @@ -51,6 +57,14 @@
public final class TezUtils {

private static final Logger LOG = LoggerFactory.getLogger(TezUtils.class);
private static final int PROPERTY_THRESHOLD;
private static final boolean PROPERTY_MASK;

static {
TezConfiguration c = new TezConfiguration();
Copy link
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)

Copy link
Contributor Author

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.

PROPERTY_THRESHOLD = c.getInt(TEZ_LOGGING_PROPERTY_SIZE_THRESHOLD, TEZ_LOGGING_PROPERTY_SIZE_THRESHOLD_DEFAULT);
PROPERTY_MASK = c.getBoolean(TEZ_LOGGING_PROPERTY_MASK, TEZ_LOGGING_PROPERTY_MASK_DEFAULT);
}

private TezUtils() {}

Expand Down Expand Up @@ -211,10 +225,19 @@ public static void populateConfProtoFromEntries(Iterable<Map.Entry<String, Strin
kvp.setKey(key);
kvp.setValue(val);
confBuilder.addConfKeyValues(kvp);
logEntryIfLarge(key, val);
} else {
LOG.debug("null value for key={}. Skipping.", key);
}
}
}

private static void logEntryIfLarge(String key, String value) {
if (value.length() > PROPERTY_THRESHOLD) {
LOG.warn("Property '{}' is unusually big ({} bytes); large payload may lead to OOM.", key, value.length());
if (!PROPERTY_MASK) {
LOG.warn("Large property '{}': {}", key, value);
}
}
}
}
20 changes: 20 additions & 0 deletions tez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1580,6 +1580,26 @@ public TezConfiguration(boolean loadDefaults) {
TEZ_PREFIX + "generate.debug.artifacts";
public static final boolean TEZ_GENERATE_DEBUG_ARTIFACTS_DEFAULT = false;

/**
* Int value. Property size threshold (in bytes) for logging during payload serialization. Properties exceeding the
* threshold are considered unusually large and potentially problematic thus they should be logged.
*/
@ConfigurationScope(Scope.VERTEX)
@ConfigurationProperty(type="integer")
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;
/**
Copy link
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d8215af

* Boolean value. Whether property masking is enabled for logging. Properties may contain sensitive user information
* such as passwords, credentials, secrets, etc., so they shouldn't be logged unconditionally. When masking is
* enabled, the property value (content) is not displayed in the logs.
*/
@ConfigurationScope(Scope.VERTEX)
@ConfigurationProperty
public static final String TEZ_LOGGING_PROPERTY_MASK =
TEZ_PREFIX + "logging.property.mask";
public static final boolean TEZ_LOGGING_PROPERTY_MASK_DEFAULT = true;

/**
* Set of tasks for which specific launch command options need to be added.
* Format: "vertexName[csv of task ids];vertexName[csv of task ids].."
Expand Down
Loading