-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Initial changes for 1911 #146
Conversation
@@ -99,16 +100,46 @@ private void stopAndUninstallBundle(final Bundle bundle, final String pluginName | |||
registry.remove(pluginName); | |||
} | |||
|
|||
public void startBundles() { | |||
public void startBundles(final List<String> mandatoryPlugins) { |
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.
We could use an Iterable
here - and the caller could pass a Set
(no duplicate)
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.
Fixed
boolean allMandatoryPluginsStarted = true; | ||
for (String pluginName: mandatoryPlugins) { | ||
if(!pluginsStarted.contains(pluginName)) { | ||
log.error("Mandatory plugin {} not started",pluginName); //TODO_1911 - Exit 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.
You could 'warn' instead and as discussed caller will take action - you can remove the 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.
Made changes to warn
and throw an exception
} | ||
|
||
private void checkIfMandatoryPluginsAreStarted(List<String> pluginsStarted, List<String> mandatoryPlugins){ |
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.
Nit: final
keyword
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.
Fixed
allMandatoryPluginsStarted = false; | ||
} | ||
} | ||
if(allMandatoryPluginsStarted) { |
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.
Nit: space
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.
Fixed
@@ -98,8 +99,10 @@ public void initialize() { | |||
|
|||
@LifecycleHandlerType(LifecycleHandlerType.LifecycleLevel.START_PLUGIN) | |||
public void start() { | |||
final String mandatoryPlugins = osgiConfig.getMandatoryPluginsList(); | |||
final List<String> mandatoryPluginsList = mandatoryPlugins != null && !mandatoryPlugins.isEmpty() ? List.of(mandatoryPlugins.split(",")) : Collections.emptyList(); |
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 forgot what we do for other similar types of properties: Do we need to split taking into account whitespaces, e.g. "\\s*,\\s*"
(not tested) ?
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.
Fixed to split by taking whitespaces into account.
@Config("org.killbill.billing.plugin.mandatory.plugins") | ||
@Description("List of mandatory plugins") | ||
@Default("") | ||
public String getMandatoryPluginsList(); |
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 we return a Set
?
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 wasn't sure how to return a Set
from OSGIConfig
(all the methods in this class return a String
). I'cev made changes to convert the String to a Set
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.
We have some examples already like here
@@ -53,10 +52,14 @@ public class DefaultLifecycle implements Lifecycle { | |||
// See https://github.com/killbill/killbill-commons/issues/143 | |||
private final Map<LifecycleLevel, SortedSet<LifecycleHandler<? extends KillbillService>>> handlersByLevel; | |||
|
|||
private static final String EXIT_ON_LIFECYCLE_ERROR_PROPERTY = "org.killbill.server.exit.on.lifecycle.error"; |
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.
Ideally, we should expose this under a KillbillPlatformConfig
class - unfortunately, we cannot use KillbillServerConfig
as it is not available from the lifecycle
module. One option would be to create a LifecycleConfig
and expose the property. I suggest we could look at this on a subsequent PR.
@Config("org.killbill.billing.plugin.mandatory.plugins") | ||
@Description("List of mandatory plugins") | ||
@Default("") | ||
public String getMandatoryPluginsList(); |
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.
We have some examples already like here
No description provided.