TEZ-4527: Add generic and pluggable hooks for DAGs and task attempts#324
Merged
abstractdog merged 8 commits intoapache:masterfrom Dec 22, 2024
Merged
TEZ-4527: Add generic and pluggable hooks for DAGs and task attempts#324abstractdog merged 8 commits intoapache:masterfrom
abstractdog merged 8 commits intoapache:masterfrom
Conversation
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
| @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"; |
Contributor
Author
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.
Contributor
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
Contributor
There was a problem hiding this comment.
I think you can remove the NoopTezThreadDumpHelper
Contributor
Author
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 | |||
Contributor
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
Contributor
|
@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
Contributor
Author
|
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
| "path: {}", duration, basePath); | ||
| } | ||
|
|
||
| public TezThreadDumpHelper() { |
Contributor
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?
Contributor
Author
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
Contributor
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
| @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"; |
Contributor
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
Contributor
thanks @okumin , this is very close, just left 2 comments |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
Thank you. This change is so helpful for us |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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