TEZ-4527: Add generic and pluggable hooks for DAGs and task attempts #324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
okumin
changed the title
okumin
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
okumin
commented
Dec 18, 2023
| */ | ||
| @ConfigurationScope(Scope.DAG) | ||
| @ConfigurationProperty | ||
| public static final String TEZ_THREAD_DUMP_INTERVAL = "tez.thread.dump.interval"; | ||
| public static final String TEZ_THREAD_DUMP_INTERVAL_DEFAULT = "0ms"; | ||
| public static final String TEZ_THREAD_DUMP_INTERVAL_DEFAULT = "100ms"; |
There was a problem hiding this comment.
If we introduce pluggable hooks, I think we can change the default value. We may remove NOOP_TEZ_THREAD_DUMP_HELPER, too.
There was a problem hiding this comment.
makes sense
I think for package clarity's sake, all the hook related configs can go a namespace that implies they're hooks:
tez.hook.thread.dump.internal
also:
TEZ_HOOK_THREAD_DUMP_INTERVAL
There was a problem hiding this comment.
I think you can remove the NoopTezThreadDumpHelper
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
abstractdog
reviewed
Dec 21, 2024
| @@ -2207,7 +2210,9 @@ public Void run() throws Exception { | |||
| } | |||
|
|
|||
| // Check if the thread dump service is up in any case, if yes attempt a shutdown | |||
There was a problem hiding this comment.
remove thread dump helper related comment, and change to a more generic one that tells we're about to stop hooks if they are running in any case
|
@okumin : thanks for the patch, nice refactor, only minor comments, other than that, it looks good to me! |
okumin
force-pushed
the
TEZ-4527-hook
branch
from
e33316d to
943012a
Compare
|
Thanks. I think all the points follow your suggestions. I rebased the branch as the original one was already obsolete |
|
🎊 +1 overall
This message was automatically generated. |
abstractdog
reviewed
Dec 22, 2024
| @@ -70,21 +71,17 @@ private TezThreadDumpHelper(long duration, Configuration conf) throws IOExceptio | |||
| "path: {}", duration, basePath); | |||
| } | |||
|
|
|||
| public TezThreadDumpHelper() { | |||
There was a problem hiding this comment.
hm, cannot recall what the purpose was of this constructor, does reflection work without this explicitly defined?
I'm afraid that as there is private parameterized constructor, class.newInstance() throws an InstantiationException, doesn't it?
There was a problem hiding this comment.
It is originally needed to instantiate
I think the class is not constructed in a reflective way, or it doesn't assume it's reflectively operated. I slightly updated the modifiers to make sure it
61d8249
There was a problem hiding this comment.
right, I was wrong, the hooks are created by reflection, but the TezThreadDumpHelper is not
helper = TezThreadDumpHelper.getInstance(conf).start(id.toString());
abstractdog
reviewed
Dec 22, 2024
| */ | ||
| @ConfigurationScope(Scope.DAG) | ||
| @ConfigurationProperty | ||
| public static final String TEZ_THREAD_DUMP_INTERVAL = "tez.thread.dump.interval"; | ||
| public static final String TEZ_THREAD_DUMP_INTERVAL_DEFAULT = "0ms"; | ||
| public static final String TEZ_HOOK_THREAD_DUMP_INTERVAL = "tez.hook.thread.dump.interval"; |
There was a problem hiding this comment.
@okumin : I'm terribly sorry, I just realized that changing this causes more problems than benefits (changing config opts from one release to another), the class name also doesn't have "hook" in it, so it's fine to have this as "tez.thread.dump.interval", are you fine with changing back? TEZ_THREAD_DUMP_INTERVAL was also fine from this point of view
There was a problem hiding this comment.
np. I renamed them back
293ce63
thanks @okumin , this is very close, just left 2 comments |
|
🎊 +1 overall
This message was automatically generated. |
|
Thank you. This change is so helpful for us |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/TEZ-4527