Uh oh!
There was an error while loading. Please reload this page.
TEZ-4039: Tez should inject dag id, query id into MDC - #98
Conversation
| */ | ||
| @ConfigurationScope(Scope.AM) | ||
| @ConfigurationProperty | ||
| public static final String TEZ_LOG_PATTERN_LAYOUT_AM = TEZ_PREFIX + "log.pattern.layout.am"; |
| */ | ||
| @ConfigurationScope(Scope.AM) | ||
| @ConfigurationProperty | ||
| public static final String TEZ_LOG_PATTERN_LAYOUT_TASK = TEZ_PREFIX + "log.pattern.layout.task"; |
| * By this option, user can easily override the logging pattern which is applied in | ||
| * TezContainerLogAppender in tasks, regardless of the environmental settings. | ||
| */ | ||
| @ConfigurationScope(Scope.AM) |
There was a problem hiding this comment.
Scope should be Vertex since this task specific config
| public static final String TEZ_MDC_CUSTOM_KEYS_DEFAULT = ""; | ||
| /** | ||
| * Comma separated list of keys, which can used for defining keys in Configuration. Tez will try |
There was a problem hiding this comment.
no, these are still keys, but keys in Configuration
tez.mdc.custom.keys=queryId
tez.mdc.custom.keys.values.from=hive.query.id <- here we are
I'm trying to clarify this in javadoc
| public static final String TEZ_MDC_CUSTOM_KEYS_DEFAULT = ""; | ||
| /** | ||
| * Comma separated list of keys, which can used for defining keys in Configuration. Tez will try |
There was a problem hiding this comment.
(same comment as above)
| /** | ||
| * NonClonableHashtable is a special class for hacking the log4j MDC context. By design, log4j's | ||
| * MDC uses a ThreadLocalMap, which clones parent thread's context before propagating it to child |
There was a problem hiding this comment.
Isn't this configurable via isThreadContextMapInheritable system property?
There was a problem hiding this comment.
unfortunately, isThreadContextMapInheritable is slightly different from what I needed:
Set the system property `log4j2.isThreadContextMapInheritable` to `true` to enable child threads to inherit the Thread Context Map.
this is used for inheritance, which is needed, child thread inherits parent's context, this is fine, the problem is that it inherits and clones the map, which is against my implementation...
my implementation makes it possible to define a global MDC context in DAGAppMaster and TezChild in the main thread, and all threads inherit that, but when a new dag (DAGAppMaster) or task attempt (TezChild) comes, modifying the global thread's context should have an effect on all threads' context, but with cloning, it won't have:
https://github.com/apache/log4j/blob/trunk/src/main/java/org/apache/log4j/helpers/ThreadLocalMap.java#L34-L41
instead, I choose to define 1 context in the main thread and propagating it to all child threads (which is automatic due to ThreadLocalMap behavior by default), but I only need to init the logging context once for every dag/taskattempt. Without cloning, a single change will change the MDC contents of all threads in the JVM.
abstractdog
commented
Feb 6, 2021
thanks for the review @prasanthj, I tried to address everything you mentioned |
Change-Id: I4ba3a5ff69ab680f7f65b4dfd438e1ab82d182bc
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| */ | ||
| @ConfigurationScope(Scope.VERTEX) | ||
| @ConfigurationProperty | ||
| public static final String TEZ_LOG_PATTERN_LAYOUT_TASK = TEZ_TASK_PREFIX + "log.pattern.layout"; |
There was a problem hiding this comment.
Is there any default value for this? (e.g If the user wants to reset it back to old value)
There was a problem hiding this comment.
good catch, user should be able to turn this off by setting it to an empty string, I'm reflecting this in the configuration
| patternConfig.set(TezConfiguration.TEZ_TASK_LOG_LEVEL, "debug"); | ||
| patternConfig.set(TezConfiguration.TEZ_LOG_PATTERN_LAYOUT_AM, | ||
| "%d{ISO8601} [%p] [%t (queryId=%X{queryId} dag=%X{dagId})] |%c{2}|: %m%n"); | ||
| patternConfig.set(TezConfiguration.TEZ_LOG_PATTERN_LAYOUT_TASK, |
There was a problem hiding this comment.
Can you check for the case when the value can be reset back to original value? Plz refer to my comment earlier in TezConfiguration.
There was a problem hiding this comment.
yeah, added testcase
| mdcContext.set(new NonClonableHashtable<String, String>()); | ||
| try { | ||
| final Constructor<?>[] constructors = org.apache.log4j.MDC.class.getDeclaredConstructors(); |
There was a problem hiding this comment.
log4j, intentionally, the whole hack implemented in this method is log4j specific
| // don't want to fail on incorrect mdc key settings, but warn in app logs | ||
| if (mdcKey.isEmpty() || mdcKeysValuesFrom.length < i + 1) { | ||
| LOG.warn("cannot set mdc key: {}", mdcKey); | ||
| break; |
There was a problem hiding this comment.
Did you mean break or continue here?
There was a problem hiding this comment.
if we hit this it means that mdcKey is most probably empty, so we had:
tez.mdc.custom.keys=
as it's very unlikely that the user set something like this:
tez.mdc.custom.keys=,something_which_matters
if we split string by commas and find empty string, it makes no sense to loop further, or at least I cannot figure out a valid use-case
thanks a lot, fixed in https://github.com/apache/tez/pull/98/commits/01adba3cfca7bbd27d6bbacbbd9bb3041d03ab3cplease let me know if you're fine with them (and my comments) |
This comment was marked as outdated.
This comment was marked as outdated.
tez-yetus
commented
Oct 25, 2022
🎊 +1 overall
This message was automatically generated. |
rbalamohan
commented
Oct 25, 2022
LGTM. +1 |
No description provided.