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
integrate flow trigger service with az web server #1633
integrate flow trigger service with az web server #1633
Conversation
@@ -97,6 +98,11 @@ public FlowTriggerService(final FlowTriggerDependencyPluginManager pluginManager | |||
this.flowTriggerInstanceLoader = flowTriggerInstanceLoader; | |||
} | |||
|
|||
public void start() throws FlowTriggerDependencyPluginException { |
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.
I'm seeing this variable List<TriggerInstance> runningTriggers
exist in this class. In the long run, AZ web server will be stateless, so state should be removed eventually. For now, It might be ok to stay with this approach. Please come up with the solution to address it.
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.
not sure what you mean here by stateless. let's discuss more in person.
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.
stateless
means more than one AZ webservers will serve web requests, and each server should not contain states internally. Could you think of any solution?
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.
then how would quartz scheduler handle this case? E.x, how does quartz know which schedule is served by which web server? We can follow the same way.
private void loadAllDependencyPlugins() throws FlowTriggerDependencyPluginException { | ||
/** | ||
* Initialize all dependency plugins. | ||
* todo chengren311: Current design aborts loadAllPlugins if any of the plugin fails to be |
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.
That's good Todo
list!
try { | ||
dependencyPluginDir = props.getString(ConfigurationKeys.DEPENDENCY_PLUGIN_DIR); | ||
} catch (final Exception ex) { | ||
dependencyPluginDir = null; |
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.
Why don't we throw exception here
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.
since it will fail the azkaban web server startup if DEPENDENCY_PLUGIN_DIR is not specified in the config file. But once the corresponding DEPENDENCY_PLUGIN_DIR is created on az machines and specified in the config file, i will add null check here.
if (this.props.getBoolean(ConfigurationKeys.ENABLE_FLOW_TRIGGER, false)) { | ||
// flow trigger service throws exception when any dependency plugin fails to be initialized | ||
// (e.x if it's kafka dependency and kafka is down). In this case if azkaban admin still | ||
// wishes to start azkaban web server, she can disable flow trigger in the az config file and |
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.
she
?
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.
she as the gender-neutral pronoun
This PR injects flow trigger service(#1627) into azkaban web server. Flow trigger service will start if enabled in the config file. Upon start, flow trigger service initializes all dependency plugins and recover running trigger instance from last run status.