-
Notifications
You must be signed in to change notification settings - Fork 440
TEZ-4647: Refactor plugin management from DAGAppMaster #428
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.
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
Conversation
997d249 to
a38ec96
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a38ec96 to
b0d9e76
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b0d9e76 to
3070c74
Compare
This comment was marked as outdated.
This comment was marked as outdated.
3070c74 to
fd835f1
Compare
fd835f1 to
f0c4020
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| public static void addDescriptor(List<NamedEntityDescriptor> list, BiMap<String, Integer> pluginMap, | ||
| NamedEntityDescriptor namedEntityDescriptor) { | ||
| list.add(namedEntityDescriptor); | ||
| pluginMap.put(list.get(list.size() - 1).getEntityName(), list.size() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do getLast instead of size -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, absolutely
|
🎊 +1 overall
This message was automatically generated. |
|
thanks @ayushtkn for the review, I'm merging this soon after addressing your comment |
From Jira:
The proposal is to create a PluginManager to offload much of the plugin management code from the already overloaded DAGAppMaster class (including its static methods).
This issue was discovered while working on TEZ-4007, which only exacerbated the situation with the addition of AMExtensions (not AMExtensions themselves, but rather the original code in DAGAppMaster). Let’s clean this up first before proceeding with TEZ-4007.
Testing done with affected unit tests
mvn install -Dtest=TestTaskSchedulerManager,TestVertexImpl2,TestDAGAppMaster -pl ./tez-dag