[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user asfgit closed the pull request at: https://github.com/apache/brooklyn-server/pull/852 ---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/852#discussion_r144296209
--- Diff:
api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java ---
@@ -213,6 +213,17 @@
void addItem(CatalogItem item);
/**
+ * adds the given items to the catalog, similar to {@link
#reset(Collection)} but where it
+ * just adds without removing the existing content. Note this is very
different from
+ * {@link #addItem(CatalogItem)}, which adds to the 'manual' catalog.
+ *
+ * @since 0.13.0 (only for legacy backwards compatibility)
+ * @deprecated since 0.13.0; instead use bundles in persisted state!
--- End diff --
Same as above
---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/852#discussion_r144296911
--- Diff:
core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java
---
@@ -1808,6 +1808,42 @@ public void addItem(CatalogItem item) {
}
@Override @Deprecated /** @deprecated see super */
+public void addCatalogLegacyItemsOnRebind(Iterable> items) {
+addCatalogLegacyItemsOnRebind(items, true);
+}
+
+private void addCatalogLegacyItemsOnRebind(Iterable> items, boolean failOnLoadError) {
--- End diff --
This method doesn't seem to be reused so I would move it's body directly
under `addCatalogLegacyItemsOnRebind(Iterable>
items)`
---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user tbouron commented on a diff in the pull request: https://github.com/apache/brooklyn-server/pull/852#discussion_r144315009 --- Diff: launcher/src/test/resources/rebind-test-empty-catalog.bom --- @@ -0,0 +1,25 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +brooklyn.catalog: + version: "test-version" + itemType: entity + items: + + # Do not scan classpath with for classes with a @Catalog annotation + - scanJavaAnnotations: false --- End diff -- The `scanJavaAnnotations` has been removed if I recall correctly, this can probably be removed then ---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/852#discussion_r144315802
--- Diff:
launcher/src/test/java/org/apache/brooklyn/launcher/AbstractBrooklynLauncherRebindTest.java
---
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.launcher;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.zip.ZipEntry;
+
+import javax.annotation.Nullable;
+
+import org.apache.brooklyn.api.catalog.BrooklynCatalog;
+import org.apache.brooklyn.api.catalog.CatalogItem;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests;
+import org.apache.brooklyn.util.collections.MutableMap;
+import org.apache.brooklyn.util.core.ResourceUtils;
+import org.apache.brooklyn.util.core.osgi.BundleMaker;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.osgi.VersionedName;
+import org.apache.brooklyn.util.text.Identifiers;
+import org.apache.brooklyn.util.time.Duration;
+import org.apache.commons.collections.IteratorUtils;
+import org.osgi.framework.Constants;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+
+import com.google.common.base.Charsets;
+import com.google.common.base.Function;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.io.Files;
+
+public abstract class AbstractBrooklynLauncherRebindTest {
+
+protected List launchers =
Lists.newCopyOnWriteArrayList();
+protected List tmpFiles = new ArrayList<>();
+
+protected String persistenceDir;
+
+@BeforeMethod(alwaysRun=true)
+public void setUp() throws Exception {
+launchers.clear();
+tmpFiles.clear();
+persistenceDir = newTempPersistenceContainerName();
+}
+
+@AfterMethod(alwaysRun=true)
+public void tearDown() throws Exception {
+for (File file : tmpFiles) {
+if (file.exists()) file.delete();
+}
+for (BrooklynLauncher launcher : launchers) {
+launcher.terminate();
+}
+launchers.clear();
+if (persistenceDir != null) Os.deleteRecursively(persistenceDir);
+}
+
+protected boolean useOsgi() {
+return false;
+}
+
+protected boolean reuseOsgi() {
+return true;
+}
+
+protected BrooklynLauncher newLauncherForTests() {
+return newLauncherForTests(PersistMode.AUTO,
HighAvailabilityMode.DISABLED);
+}
+
+protected BrooklynLauncher newLauncherForTests(PersistMode
persistMode, HighAvailabilityMode haMode) {
+BrooklynLauncher launcher = BrooklynLauncher.newInstance()
+
.brooklynProperties(LocalManagementContextForTests.builder(true).setOsgiEnablementAndReuse(useOsgi(),
reuseOsgi()).buildProperties())
+.persistMode(persistMode)
+.highAvailabilityMode(haMode)
+.persistPeriod(Duration.millis(10))
+.haHeartbeatPeriod(Duration.millis(10))
+.persistenceDir(persistenceDir)
+.webconsole(false);
+launchers.add(launcher);
+return launcher;
+}
+
+protected String newTempPersistenceContainerName() {
+File persistenceDirF = Files.createTempDir();
+Os.deleteOnExitRe
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/852#discussion_r144296174
--- Diff:
api/src/main/java/org/apache/brooklyn/api/catalog/BrooklynCatalog.java ---
@@ -213,6 +213,17 @@
void addItem(CatalogItem item);
/**
+ * adds the given items to the catalog, similar to {@link
#reset(Collection)} but where it
+ * just adds without removing the existing content. Note this is very
different from
+ * {@link #addItem(CatalogItem)}, which adds to the 'manual' catalog.
+ *
+ * @since 0.13.0 (only for legacy backwards compatibility)
--- End diff --
s/0.13.0/1.0.0/
---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/852#discussion_r144297930
--- Diff:
core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogInitialization.java
---
@@ -128,164 +146,255 @@ public void setFailOnStartupErrors(boolean
startupFailOnCatalogErrors) {
this.failOnStartupErrors = startupFailOnCatalogErrors;
}
-public CatalogInitialization
addPopulationCallback(Function callback) {
-callbacks.add(callback);
-return this;
-}
-
public ManagementContextInternal getManagementContext() {
-return Preconditions.checkNotNull(managementContext, "management
context has not been injected into "+this);
+return checkNotNull(managementContext, "management context has not
been injected into "+this);
}
/** Returns true if the canonical initialization has completed,
* that is, an initialization which is done when a node is rebinded as
master
* (or an initialization done by the startup routines when not running
persistence);
* see also {@link #hasRunAnyInitialization()}. */
-public boolean hasRunFinalInitialization() { return
hasRunFinalInitialization; }
+private boolean hasRunFinalInitialization() {
+return hasRunFinalInitialization;
+}
-/** Returns true if an official initialization has run,
- * even if it was a transient run, e.g. so that the launch sequence
can tell whether rebind has triggered initialization */
-public boolean hasRunOfficialInitialization() { return
hasRunFinalInitialization || hasRunTransientOfficialInitialization; }
+private boolean hasRunInitialCatalogInitialization() {
+return hasRunInitialCatalogInitialization;
--- End diff --
Shouldn't be as below, i.e. `return hasRunFinalInitialization ||
hasRunInitialCatalogInitialization;` ?
---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/852#discussion_r142992671
--- Diff:
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindCatalogEntityTest.java
---
@@ -1,153 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.brooklyn.core.mgmt.rebind;
-
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertNotSame;
-
-import java.io.Closeable;
-import java.net.URL;
-import java.util.List;
-
-import org.apache.brooklyn.api.entity.Application;
-import org.apache.brooklyn.api.entity.EntitySpec;
-import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
-import org.apache.brooklyn.core.catalog.internal.CatalogInitialization;
-import org.apache.brooklyn.core.config.ConfigKeys;
-import org.apache.brooklyn.core.entity.AbstractApplication;
-import org.apache.brooklyn.core.entity.Entities;
-import org.apache.brooklyn.core.entity.EntityInternal;
-import org.apache.brooklyn.core.entity.StartableApplication;
-import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
-import org.apache.brooklyn.core.sensor.Sensors;
-import org.apache.brooklyn.test.support.TestResourceUnavailableException;
-import org.apache.brooklyn.util.core.javalang.UrlClassLoader;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.testng.annotations.BeforeMethod;
-import org.testng.annotations.Test;
-
-import com.google.common.base.Function;
-
-public class RebindCatalogEntityTest extends
RebindTestFixture {
--- End diff --
See description in this individual commit:
```
Was testing use of catalog class loader, not OSGi, with catalog item
requiring custom class loader being added programmatically via
catalog.addItem() and then fiddling with catalog on rebind to add it
back in!
No user should be doing that; the test is not something we want to
support.
```
---
[GitHub] brooklyn-server pull request #852: Merge initial catalog and persisted catal...
GitHub user aledsage opened a pull request: https://github.com/apache/brooklyn-server/pull/852 Merge initial catalog and persisted catalog When starting Brooklyn, the catalog is now populated by merging the initial catalog (from the default .bom file) with the persisted catalog. This PR is pretty much a re-write of the `CatalogInitialization` class to do this. --- It also cleans up some code, some code paths and some strange things we've previously supported. Of particular interest... **No longer auto-populates catalog on `localManagementContext.getCatalog()`** Previously `localManagementContext.getCatalog()` would cause the catalog to be populated if it had not already been done so. This was important for tests that did not go through the full intialization lifecycle. I've moved that into the class `LocalManagementContextForTests` - we should restrict the code-paths for production usage to just those we expect! The consequence is that any tests relying on it must create the `LocalManagementContextForTests` rather than a full-blown `LocalManagementContext` (almost all Brooklyn tests do this already), but this might affect downstream projects. **Fewer ways catalog initialization can happen** We now only support 2 production ways of populating the catalog: * `populateInitialCatalogOnly`: called if persistence is disabled * `populateInitialAndPersistedCatalog`: called on rebind The second way can be called multiple times (e.g. if repeatedly doing it for `hot-standby`, or if subsequently promoting to `master`). The method `unofficialPopulateInitialCatalog` is to be only called by tests (though unfortunately not enforced as such). **Tidy BasicLauncher's persistence initialization** * `confirmCatalog` is moved from `Main` to `CatalogInitialization` * `BasicLauncher.checkConfiguration` now checks that we never enable HA mode if persistence is disabled. Moves handling of non-persistence out of `initPersistence`. You can merge this pull request into a Git repository by running: $ git pull https://github.com/aledsage/brooklyn-server catalog-init-merge-initial-and-persisted Alternatively you can review and apply these changes as the patch at: https://github.com/apache/brooklyn-server/pull/852.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #852 commit eea6904bc72e33135ba2b2b55bbdb79c457f8c8c Author: Aled Sage Date: 2017-10-02T17:30:10Z Merge initial catalog and persisted catalog commit 16d6c7364ababbd92009d54a65f40be6dd0db819 Author: Aled Sage Date: 2017-10-04T09:36:19Z Delete RebindCatalogEntityTest Was testing use of catalog class loader, not OSGi, with catalog item requiring custom class loader being added programmatically via catalog.addItem() and then fiddling with catalog on rebind to add it back in! No user should be doing that; the test is not something we want to support. ---
