jkevan commented on code in PR #432:
URL: https://github.com/apache/unomi/pull/432#discussion_r891392822
##########
lifecycle-watcher/src/main/java/org/apache/unomi/lifecycle/BundleWatcherImpl.java:
##########
@@ -223,50 +269,109 @@ private void displayLogsForInactiveServices() {
});
}
+ private void prepareGraphQLFeatureToInstall() {
+ String installGraphQLFeature =
bundleContext.getProperty("org.apache.unomi.graphql.feature.activated");
+ boolean graphQLToInstall =
StringUtils.isNotBlank(installGraphQLFeature) &&
installGraphQLFeature.equals("true");
+ if (graphQLToInstall) {
Review Comment:
```suggestion
if
(Boolean.parseBoolean(bundleContext.getProperty("org.apache.unomi.graphql.feature.activated")))
{
```
##########
package/pom.xml:
##########
@@ -377,9 +377,8 @@
<feature>unomi-router-karaf-feature</feature>
<feature>unomi-web-tracker-karaf-kar</feature>
<feature>unomi-groovy-actions</feature>
- <feature>cdp-graphql-feature</feature>
<feature>unomi-rest-ui</feature>
- <feature>unomi-graphql-ui</feature>
+ <feature>cdp-graphql-feature-denpendencies</feature>
Review Comment:
So if I do understand correctly this feature is still boot and all the
GraphQL dependencies are still started up even if we have disabled GraphQL ?
May be this feature should be merged with the unomi graphQL implem feature
and be activated by the BundleWatcher.
##########
graphql/karaf-feature/src/main/feature/feature.xml:
##########
@@ -52,6 +53,11 @@
<bundle
start-level="80">mvn:org.eclipse.jetty/jetty-security/${jetty.websocket.version}</bundle>
<bundle
start-level="80">mvn:org.eclipse.jetty/jetty-server/${jetty.websocket.version}</bundle>
<bundle
start-level="80">mvn:org.eclipse.jetty/jetty-http/${jetty.websocket.version}</bundle>
- <bundle start-level="80"
start="false">mvn:org.apache.unomi/cdp-graphql-api-impl/${project.version}</bundle>
+ </feature>
+ <feature name="cdp-graphql-feature" description="Apache Unomi :: GraphQL
API :: Karaf Feature"
+ version="${project.version}">
+ <feature>unomi-kar</feature>
+ <bundle
start-level="80">mvn:org.apache.unomi/cdp-graphql-api-impl/${project.version}</bundle>
+ <bundle
start-level="80">mvn:org.apache.unomi/unomi-graphql-playground/${project.version}</bundle>
Review Comment:
Why not merging in a single feature ?
seem's that one doesnt make sense without the other.
##########
lifecycle-watcher/src/main/java/org/apache/unomi/lifecycle/BundleWatcher.java:
##########
@@ -30,10 +30,12 @@ public interface BundleWatcher {
* Retrieves the list of the server information objects, that include
extensions. Each object includes the
* name and version of the server, build time information and the event
types
* if recognizes as well as the capabilities supported by the system.
+ *
* @return a list of ServerInfo objects with all the server information
*/
List<ServerInfo> getServerInfos();
-
boolean isStartupComplete();
+
+ boolean allAdditionalBundleStarted();
Review Comment:
IS it necessary to have a public function about that ?
For what purpose ?
If it's necessary, please add some JavaDoc.
##########
lifecycle-watcher/src/main/java/org/apache/unomi/lifecycle/BundleWatcherImpl.java:
##########
@@ -223,50 +269,109 @@ private void displayLogsForInactiveServices() {
});
}
+ private void prepareGraphQLFeatureToInstall() {
+ String installGraphQLFeature =
bundleContext.getProperty("org.apache.unomi.graphql.feature.activated");
+ boolean graphQLToInstall =
StringUtils.isNotBlank(installGraphQLFeature) &&
installGraphQLFeature.equals("true");
+ if (graphQLToInstall) {
+ featuresToInstall.add(CDP_GRAPHQL_FEATURE);
+
requiredBundlesFromFeatures.put("org.apache.unomi.cdp-graphql-api-impl", false);
+
requiredBundlesFromFeatures.put("org.apache.unomi.graphql-playground", false);
+ }
+ }
+
+ public boolean shouldInstallAdditionalFeatures() {
+ return !featuresToInstall.isEmpty();
+ }
+
+ private void installFeatures() {
+ List<String> installedFeatures = new ArrayList<>();
+ featuresToInstall.forEach(value -> {
+ try {
+ long featureStartupTime = System.currentTimeMillis();
+ if
(!featuresService.isInstalled(featuresService.getFeature(value))) {
+ System.out.println("Installing feature " + value);
+ featuresService.installFeature(value,
EnumSet.of(FeaturesService.Option.NoAutoRefreshManagedBundles,
+
FeaturesService.Option.NoAutoRefreshUnmanagedBundles,
FeaturesService.Option.NoAutoRefreshBundles));
+ logger.info("Feature {} successfully installed in {} ms",
value, System.currentTimeMillis() - featureStartupTime);
+ }
+ installedFeatures.add(value);
+ } catch (Exception e) {
+ logger.error("Error when installing {} feature", value, e);
+ }
+ });
+ installedFeatures.forEach(value -> featuresToInstall.remove(value));
+ }
+
+ private void startScheduler() {
+ if (scheduledFuture == null || scheduledFuture.isCancelled()) {
+ TimerTask task = new TimerTask() {
+ @Override
+ public void run() {
+ displayLogsForInactiveBundles(requiredBundles);
+ displayLogsForInactiveServices();
+ checkStartupComplete();
+ }
+ };
+ scheduledFuture = scheduler
+ .scheduleWithFixedDelay(task,
checkStartupStateRefreshInterval, checkStartupStateRefreshInterval,
TimeUnit.SECONDS);
+ }
+ }
+
+ private void startSchedulerForAdditionalBundles() {
+ if (scheduledFutureAdditionalBundles == null ||
scheduledFutureAdditionalBundles.isCancelled()) {
+ TimerTask task = new TimerTask() {
+ @Override
+ public void run() {
+ if (shouldInstallAdditionalFeatures() &&
!installingFeatureStarted) {
+ installingFeatureStarted = true;
+ installFeatures();
+ }
+ displayLogsForInactiveBundles(requiredBundlesFromFeatures);
+ checkStartupComplete();
+ }
+ };
+ scheduledFutureAdditionalBundles = scheduler
+ .scheduleWithFixedDelay(task,
checkStartupStateRefreshInterval, checkStartupStateRefreshInterval,
TimeUnit.SECONDS);
+ }
+ }
Review Comment:
The code of this function could be reduce to the task it self being
different.
(In case we reuse the same ScheduledFuture variable like suggested below.)
Or other option, just use one function here and pass everything as parameter:
- installFeature: bool -> false first time, true second time
- inactiveBundlesToWatch: requiredBundles first time,
requiredBundlesFromFeatures second time
- checkInactiveServices: bool -> true first time, false second time
Doing so we can:
- reuse existing ScheduledFuture variable, because both cannot be start in
parallel, the feature task is started after the normal bundle task is ended.
- And we can have a generic task that is able to handle both cases depending
on the parameter listed previously.
##########
lifecycle-watcher/src/main/java/org/apache/unomi/lifecycle/BundleWatcherImpl.java:
##########
@@ -223,50 +269,109 @@ private void displayLogsForInactiveServices() {
});
}
+ private void prepareGraphQLFeatureToInstall() {
+ String installGraphQLFeature =
bundleContext.getProperty("org.apache.unomi.graphql.feature.activated");
+ boolean graphQLToInstall =
StringUtils.isNotBlank(installGraphQLFeature) &&
installGraphQLFeature.equals("true");
+ if (graphQLToInstall) {
+ featuresToInstall.add(CDP_GRAPHQL_FEATURE);
+
requiredBundlesFromFeatures.put("org.apache.unomi.cdp-graphql-api-impl", false);
+
requiredBundlesFromFeatures.put("org.apache.unomi.graphql-playground", false);
+ }
+ }
+
+ public boolean shouldInstallAdditionalFeatures() {
+ return !featuresToInstall.isEmpty();
+ }
+
+ private void installFeatures() {
+ List<String> installedFeatures = new ArrayList<>();
+ featuresToInstall.forEach(value -> {
+ try {
+ long featureStartupTime = System.currentTimeMillis();
+ if
(!featuresService.isInstalled(featuresService.getFeature(value))) {
+ System.out.println("Installing feature " + value);
+ featuresService.installFeature(value,
EnumSet.of(FeaturesService.Option.NoAutoRefreshManagedBundles,
+
FeaturesService.Option.NoAutoRefreshUnmanagedBundles,
FeaturesService.Option.NoAutoRefreshBundles));
+ logger.info("Feature {} successfully installed in {} ms",
value, System.currentTimeMillis() - featureStartupTime);
+ }
+ installedFeatures.add(value);
+ } catch (Exception e) {
+ logger.error("Error when installing {} feature", value, e);
+ }
+ });
+ installedFeatures.forEach(value -> featuresToInstall.remove(value));
+ }
+
+ private void startScheduler() {
+ if (scheduledFuture == null || scheduledFuture.isCancelled()) {
+ TimerTask task = new TimerTask() {
+ @Override
+ public void run() {
+ displayLogsForInactiveBundles(requiredBundles);
+ displayLogsForInactiveServices();
+ checkStartupComplete();
+ }
+ };
+ scheduledFuture = scheduler
+ .scheduleWithFixedDelay(task,
checkStartupStateRefreshInterval, checkStartupStateRefreshInterval,
TimeUnit.SECONDS);
+ }
+ }
+
+ private void startSchedulerForAdditionalBundles() {
+ if (scheduledFutureAdditionalBundles == null ||
scheduledFutureAdditionalBundles.isCancelled()) {
+ TimerTask task = new TimerTask() {
+ @Override
+ public void run() {
+ if (shouldInstallAdditionalFeatures() &&
!installingFeatureStarted) {
+ installingFeatureStarted = true;
+ installFeatures();
+ }
+ displayLogsForInactiveBundles(requiredBundlesFromFeatures);
+ checkStartupComplete();
+ }
+ };
+ scheduledFutureAdditionalBundles = scheduler
+ .scheduleWithFixedDelay(task,
checkStartupStateRefreshInterval, checkStartupStateRefreshInterval,
TimeUnit.SECONDS);
+ }
+ }
+
private void checkStartupComplete() {
if (!isStartupComplete()) {
- if (scheduledFuture == null || scheduledFuture.isCancelled()) {
- TimerTask task = new TimerTask() {
- @Override
- public void run() {
- displayLogsForInactiveBundles();
- displayLogsForInactiveServices();
- checkStartupComplete();
- }
- };
- scheduledFuture = scheduler
- .scheduleWithFixedDelay(task,
checkStartupStateRefreshInterval, checkStartupStateRefreshInterval,
TimeUnit.SECONDS);
- }
+ startScheduler();
return;
}
if (scheduledFuture != null) {
scheduledFuture.cancel(true);
scheduledFuture = null;
}
+
+ if (!allAdditionalBundleStarted()) {
+ startSchedulerForAdditionalBundles();
+ return;
+ }
+ if (scheduledFutureAdditionalBundles != null) {
+ scheduledFutureAdditionalBundles.cancel(true);
+ scheduledFutureAdditionalBundles = null;
+ }
Review Comment:
Since the previous **scheduledFuture** is finished and destroyed we could
reuse it instead of adding a new **scheduledFutureAdditionalBundles** variable ?
And the destroy code (cancel and set to null can be extracted to a function
called: **destroyScheduler**)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
