You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2014/11/07 11:26:10 UTC
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/309
Refactor rebind code, and tidy
Please merge https://github.com/apache/incubator-brooklyn/pull/308 first
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn refactor/Rebind-code-tidy
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/309.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 #309
----
commit 54bcf0cd52030e21373cbe229a28ad5a1883d1b3
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-04T16:36:33Z
Adds RebindOptions for RebindTestUtils etc
- Changes RebindTestFixture default to not checkSerializable.
- Deprecates crazy number of overloaded methods
commit 94402b2ce79e71b4e2e614999b2107a64e6f3a75
Author: Aled Sage <al...@gmail.com>
Date: 2014-11-07T10:10:33Z
BindDnsServerIntegrationTest: more logging
- Dump a backup of the memento dir on error
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/309
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20469217
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestFixture.java ---
@@ -72,6 +64,7 @@
protected T newApp;
protected ManagementContext newManagementContext;
--- End diff --
Not sure I follow. `newManagementContext ` gets terminated in `tearDown()`.
Until my changes, the `newManagementContext` would only get set in some of the methods but not others (which definitely felt like a bug).
Now everything delegates to a single rebind() implementation, which does set `new ManagementContext` - so it should always happen.
Setting it in the sub-class, or declaring your own locally-scoped variable/field, will still work but should be unnecessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-63156858
merge conflicts again, probably my fault. looks good, apart from one comment to confirm new mgmt contexts get terminated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-62982855
Have deprecated another couple of methods, and rebased against master.
Will wait for jenkins to confirm all good, and then merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20234419
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestFixture.java ---
@@ -131,95 +124,75 @@ public void tearDown() throws Exception {
origManagementContext = null;
}
- /**
- * Dumps out the persisted mementos that are at the given directory.
- *
- * Binds to the persisted state (as a "hot standby") to load the raw data (as strings), and to write out the
- * entity, location, policy, enricher, feed and catalog-item data.
- *
- * @param dir The directory containing the persisted state (e.g. {@link #mementoDir} or {@link #mementoDirBackup})
- */
- protected void dumpMementoDir(File dir) {
- LocalManagementContextForTests mgmt = new LocalManagementContextForTests(BrooklynProperties.Factory.newEmpty());
- FileBasedObjectStore store = null;
- BrooklynMementoPersisterToObjectStore persister = null;
- try {
- store = new FileBasedObjectStore(dir);
- store.injectManagementContext(mgmt);
- store.prepareForSharedUse(PersistMode.AUTO, HighAvailabilityMode.HOT_STANDBY);
- persister = new BrooklynMementoPersisterToObjectStore(store, BrooklynProperties.Factory.newEmpty(), classLoader);
- BrooklynMementoRawData data = persister.loadMementoRawData(RebindExceptionHandlerImpl.builder().build());
- List<BrooklynObjectType> types = ImmutableList.of(BrooklynObjectType.ENTITY, BrooklynObjectType.LOCATION,
- BrooklynObjectType.POLICY, BrooklynObjectType.ENRICHER, BrooklynObjectType.FEED,
- BrooklynObjectType.CATALOG_ITEM);
- for (BrooklynObjectType type : types) {
- LOG.info(type+" ("+data.getObjectsOfType(type).keySet()+"):");
- for (Map.Entry<String, String> entry : data.getObjectsOfType(type).entrySet()) {
- LOG.info("\t"+type+" "+entry.getKey()+": "+entry.getValue());
- }
- }
- } finally {
- if (persister != null) persister.stop(false);
- if (store != null) store.close();
- mgmt.terminate();
- }
- }
-
/** rebinds, and sets newApp */
protected T rebind() throws Exception {
- return rebind(true);
+ return rebind(RebindOptions.create());
}
/**
- * TODO We should (probably?!) change everywhere from asserting that they are serializable.
- * They only need to be xstream-serializable, which does not require `implements Serializable`.
+ * Checking serializable is overly strict.
+ * State only needs to be xstream-serializable, which does not require `implements Serializable`.
* Also, the xstream serializer has some special hooks that replaces an entity reference with
- * a marker for that entity, etc. Suggest we change the default {@link #rebind()} to use
- * {@code checkSerializable==false}, and deprecate this + the other overloaded methods?
+ * a marker for that entity, etc.
+ *
+ * @deprecated since 0.7.0; use {@link #rebind()} or {@link #rebind(RebindOptions)})
*/
+ @Deprecated
protected T rebind(boolean checkSerializable) throws Exception {
- // TODO What are sensible defaults?!
- return rebind(checkSerializable, false);
+ return rebind(RebindOptions.create().checkSerializable(checkSerializable));
}
-
- @SuppressWarnings("unchecked")
+
+ /**
+ * Checking serializable is overly strict.
+ * State only needs to be xstream-serializable, which does not require `implements Serializable`.
+ * Also, the xstream serializer has some special hooks that replaces an entity reference with
+ * a marker for that entity, etc.
+ *
+ * @deprecated since 0.7.0; use {@link #rebind(RebindOptions)})
+ */
+ @Deprecated
protected T rebind(boolean checkSerializable, boolean terminateOrigManagementContext) throws Exception {
- return rebind(checkSerializable, terminateOrigManagementContext, (File)null);
+ return rebind(RebindOptions.create()
+ .checkSerializable(checkSerializable)
+ .terminateOrigManagementContext(terminateOrigManagementContext));
}
-
- @Beta // temporary method while debugging; Aled will refactor all of this soon!
- @SuppressWarnings("unchecked")
- protected T rebind(boolean checkSerializable, boolean terminateOrigManagementContext, File backupDir) throws Exception {
- if (newApp!=null || newManagementContext!=null) throw new IllegalStateException("already rebound");
-
- RebindTestUtils.waitForPersisted(origApp);
- if (checkSerializable) {
- RebindTestUtils.checkCurrentMementoSerializable(origApp);
- }
- if (terminateOrigManagementContext) {
- origManagementContext.terminate();
- }
-
- if (backupDir != null) {
- FileUtil.copyDir(mementoDir, backupDir);
- FileUtil.setFilePermissionsTo700(backupDir);
- }
- newManagementContext = createNewManagementContext();
- newApp = (T) RebindTestUtils.rebind((LocalManagementContext)newManagementContext, classLoader);
- return newApp;
+ protected T rebind(RebindExceptionHandler exceptionHandler) throws Exception {
+ return rebind(RebindOptions.create().exceptionHandler(exceptionHandler));
}
+ protected T rebind(ManagementContext newManagementContext, RebindExceptionHandler exceptionHandler) throws Exception {
+ return rebind(RebindOptions.create()
+ .newManagementContext(newManagementContext)
+ .exceptionHandler(exceptionHandler));
+ }
+
@SuppressWarnings("unchecked")
- protected T rebind(RebindExceptionHandler exceptionHandler) throws Exception {
+ protected T rebind(RebindOptions options) throws Exception {
+ if (newApp!=null || newManagementContext!=null) {
+ throw new IllegalStateException("already rebound");
+ }
--- End diff --
Will callers be surprised that this exception isn't passed to a registered `exceptionHandler`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20330695
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java ---
@@ -221,71 +224,196 @@ public LocalManagementContext buildStarted() {
}
+ /**
+ * Convenience for common call; delegates to {@link #rebind(RebindOptions)}
+ */
public static Application rebind(File mementoDir, ClassLoader classLoader) throws Exception {
- return rebind(mementoDir, classLoader, null);
+ return rebind(RebindOptions.create()
+ .mementoDir(mementoDir)
+ .classLoader(classLoader));
}
--- End diff --
12 callers, most of whom should probably be refactored to extend `RebindTestFixture`.
But not doing that in this PR.
I'll leave as-is for now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-62373565
Please do not merge yet.
https://github.com/apache/incubator-brooklyn/pull/308 is merged in apache, but the changes are being really slow to hit github. I'll rebase this when the commits finally hit github.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-63385264
Waiting for jenkins to confirm, and then will merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20399054
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestFixture.java ---
@@ -72,6 +64,7 @@
protected T newApp;
protected ManagementContext newManagementContext;
--- End diff --
we've left this field here but stopped setting it in several places, replacing with locally-scoped. one of the nice things about this one though was it automatically got terminated; are things still terminated? (probably yes, but i dind't see where)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-62757343
Nice improvement. Only minor niggles.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-63448440
Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20330104
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestFixture.java ---
@@ -131,95 +124,75 @@ public void tearDown() throws Exception {
origManagementContext = null;
}
- /**
- * Dumps out the persisted mementos that are at the given directory.
- *
- * Binds to the persisted state (as a "hot standby") to load the raw data (as strings), and to write out the
- * entity, location, policy, enricher, feed and catalog-item data.
- *
- * @param dir The directory containing the persisted state (e.g. {@link #mementoDir} or {@link #mementoDirBackup})
- */
- protected void dumpMementoDir(File dir) {
- LocalManagementContextForTests mgmt = new LocalManagementContextForTests(BrooklynProperties.Factory.newEmpty());
- FileBasedObjectStore store = null;
- BrooklynMementoPersisterToObjectStore persister = null;
- try {
- store = new FileBasedObjectStore(dir);
- store.injectManagementContext(mgmt);
- store.prepareForSharedUse(PersistMode.AUTO, HighAvailabilityMode.HOT_STANDBY);
- persister = new BrooklynMementoPersisterToObjectStore(store, BrooklynProperties.Factory.newEmpty(), classLoader);
- BrooklynMementoRawData data = persister.loadMementoRawData(RebindExceptionHandlerImpl.builder().build());
- List<BrooklynObjectType> types = ImmutableList.of(BrooklynObjectType.ENTITY, BrooklynObjectType.LOCATION,
- BrooklynObjectType.POLICY, BrooklynObjectType.ENRICHER, BrooklynObjectType.FEED,
- BrooklynObjectType.CATALOG_ITEM);
- for (BrooklynObjectType type : types) {
- LOG.info(type+" ("+data.getObjectsOfType(type).keySet()+"):");
- for (Map.Entry<String, String> entry : data.getObjectsOfType(type).entrySet()) {
- LOG.info("\t"+type+" "+entry.getKey()+": "+entry.getValue());
- }
- }
- } finally {
- if (persister != null) persister.stop(false);
- if (store != null) store.close();
- mgmt.terminate();
- }
- }
-
/** rebinds, and sets newApp */
protected T rebind() throws Exception {
- return rebind(true);
+ return rebind(RebindOptions.create());
}
/**
- * TODO We should (probably?!) change everywhere from asserting that they are serializable.
- * They only need to be xstream-serializable, which does not require `implements Serializable`.
+ * Checking serializable is overly strict.
+ * State only needs to be xstream-serializable, which does not require `implements Serializable`.
* Also, the xstream serializer has some special hooks that replaces an entity reference with
- * a marker for that entity, etc. Suggest we change the default {@link #rebind()} to use
- * {@code checkSerializable==false}, and deprecate this + the other overloaded methods?
+ * a marker for that entity, etc.
+ *
+ * @deprecated since 0.7.0; use {@link #rebind()} or {@link #rebind(RebindOptions)})
*/
+ @Deprecated
protected T rebind(boolean checkSerializable) throws Exception {
- // TODO What are sensible defaults?!
- return rebind(checkSerializable, false);
+ return rebind(RebindOptions.create().checkSerializable(checkSerializable));
}
-
- @SuppressWarnings("unchecked")
+
+ /**
+ * Checking serializable is overly strict.
+ * State only needs to be xstream-serializable, which does not require `implements Serializable`.
+ * Also, the xstream serializer has some special hooks that replaces an entity reference with
+ * a marker for that entity, etc.
+ *
+ * @deprecated since 0.7.0; use {@link #rebind(RebindOptions)})
+ */
+ @Deprecated
protected T rebind(boolean checkSerializable, boolean terminateOrigManagementContext) throws Exception {
- return rebind(checkSerializable, terminateOrigManagementContext, (File)null);
+ return rebind(RebindOptions.create()
+ .checkSerializable(checkSerializable)
+ .terminateOrigManagementContext(terminateOrigManagementContext));
}
-
- @Beta // temporary method while debugging; Aled will refactor all of this soon!
- @SuppressWarnings("unchecked")
- protected T rebind(boolean checkSerializable, boolean terminateOrigManagementContext, File backupDir) throws Exception {
- if (newApp!=null || newManagementContext!=null) throw new IllegalStateException("already rebound");
-
- RebindTestUtils.waitForPersisted(origApp);
- if (checkSerializable) {
- RebindTestUtils.checkCurrentMementoSerializable(origApp);
- }
- if (terminateOrigManagementContext) {
- origManagementContext.terminate();
- }
-
- if (backupDir != null) {
- FileUtil.copyDir(mementoDir, backupDir);
- FileUtil.setFilePermissionsTo700(backupDir);
- }
- newManagementContext = createNewManagementContext();
- newApp = (T) RebindTestUtils.rebind((LocalManagementContext)newManagementContext, classLoader);
- return newApp;
+ protected T rebind(RebindExceptionHandler exceptionHandler) throws Exception {
+ return rebind(RebindOptions.create().exceptionHandler(exceptionHandler));
}
+ protected T rebind(ManagementContext newManagementContext, RebindExceptionHandler exceptionHandler) throws Exception {
+ return rebind(RebindOptions.create()
+ .newManagementContext(newManagementContext)
+ .exceptionHandler(exceptionHandler));
+ }
+
@SuppressWarnings("unchecked")
- protected T rebind(RebindExceptionHandler exceptionHandler) throws Exception {
+ protected T rebind(RebindOptions options) throws Exception {
+ if (newApp!=null || newManagementContext!=null) {
+ throw new IllegalStateException("already rebound");
+ }
--- End diff --
It's just test code, so fine to just throw I think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20330165
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindOptions.java ---
@@ -0,0 +1,92 @@
+/*
+ * 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 brooklyn.entity.rebind;
+
+import java.io.File;
+
+import brooklyn.entity.rebind.persister.PersistenceObjectStore;
+import brooklyn.management.ManagementContext;
+
+/**
+ * See {@link RebindTestFixture#rebind(RebindOptions)} and {@link RebindTestUtils#rebind(RebindOptions}}.
+ */
+public class RebindOptions {
+ boolean checkSerializable;
+ boolean terminateOrigManagementContext;
+ RebindExceptionHandler exceptionHandler;
+ ManagementContext origManagementContext;
+ ManagementContext newManagementContext;
+ File mementoDir;
+ File mementoDirBackup;
+ ClassLoader classLoader;
+ PersistenceObjectStore objectStore;
--- End diff --
`terminateOrigManagementContext` is currently defaulting to false (based on the overloaded `rebind()` methods that existed before these changes. Arguably it should be `true`. We can think about that more after this is merged, I think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20234519
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestUtils.java ---
@@ -221,71 +224,196 @@ public LocalManagementContext buildStarted() {
}
+ /**
+ * Convenience for common call; delegates to {@link #rebind(RebindOptions)}
+ */
public static Application rebind(File mementoDir, ClassLoader classLoader) throws Exception {
- return rebind(mementoDir, classLoader, null);
+ return rebind(RebindOptions.create()
+ .mementoDir(mementoDir)
+ .classLoader(classLoader));
}
--- End diff --
I'd be inclined to deprecate this version too, but no strong feelings. How common is the "common call"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20234051
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindOptions.java ---
@@ -0,0 +1,92 @@
+/*
+ * 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 brooklyn.entity.rebind;
+
+import java.io.File;
+
+import brooklyn.entity.rebind.persister.PersistenceObjectStore;
+import brooklyn.management.ManagementContext;
+
+/**
+ * See {@link RebindTestFixture#rebind(RebindOptions)} and {@link RebindTestUtils#rebind(RebindOptions}}.
+ */
+public class RebindOptions {
+ boolean checkSerializable;
+ boolean terminateOrigManagementContext;
+ RebindExceptionHandler exceptionHandler;
+ ManagementContext origManagementContext;
+ ManagementContext newManagementContext;
+ File mementoDir;
+ File mementoDirBackup;
+ ClassLoader classLoader;
+ PersistenceObjectStore objectStore;
--- End diff --
Would ideally prefer these to be private with public accessors, but I can live with it to avoid the excess noise. Explicit default values would be good, however. In particular, should `terminateOrigManagementContext` be set by defaut? Isn't that the normal thing to do?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#discussion_r20234337
--- Diff: core/src/test/java/brooklyn/entity/rebind/RebindTestFixture.java ---
@@ -131,95 +124,75 @@ public void tearDown() throws Exception {
origManagementContext = null;
}
- /**
- * Dumps out the persisted mementos that are at the given directory.
- *
- * Binds to the persisted state (as a "hot standby") to load the raw data (as strings), and to write out the
- * entity, location, policy, enricher, feed and catalog-item data.
- *
- * @param dir The directory containing the persisted state (e.g. {@link #mementoDir} or {@link #mementoDirBackup})
- */
- protected void dumpMementoDir(File dir) {
- LocalManagementContextForTests mgmt = new LocalManagementContextForTests(BrooklynProperties.Factory.newEmpty());
- FileBasedObjectStore store = null;
- BrooklynMementoPersisterToObjectStore persister = null;
- try {
- store = new FileBasedObjectStore(dir);
- store.injectManagementContext(mgmt);
- store.prepareForSharedUse(PersistMode.AUTO, HighAvailabilityMode.HOT_STANDBY);
- persister = new BrooklynMementoPersisterToObjectStore(store, BrooklynProperties.Factory.newEmpty(), classLoader);
- BrooklynMementoRawData data = persister.loadMementoRawData(RebindExceptionHandlerImpl.builder().build());
- List<BrooklynObjectType> types = ImmutableList.of(BrooklynObjectType.ENTITY, BrooklynObjectType.LOCATION,
- BrooklynObjectType.POLICY, BrooklynObjectType.ENRICHER, BrooklynObjectType.FEED,
- BrooklynObjectType.CATALOG_ITEM);
- for (BrooklynObjectType type : types) {
- LOG.info(type+" ("+data.getObjectsOfType(type).keySet()+"):");
- for (Map.Entry<String, String> entry : data.getObjectsOfType(type).entrySet()) {
- LOG.info("\t"+type+" "+entry.getKey()+": "+entry.getValue());
- }
- }
- } finally {
- if (persister != null) persister.stop(false);
- if (store != null) store.close();
- mgmt.terminate();
- }
- }
-
/** rebinds, and sets newApp */
protected T rebind() throws Exception {
- return rebind(true);
+ return rebind(RebindOptions.create());
}
/**
- * TODO We should (probably?!) change everywhere from asserting that they are serializable.
- * They only need to be xstream-serializable, which does not require `implements Serializable`.
+ * Checking serializable is overly strict.
+ * State only needs to be xstream-serializable, which does not require `implements Serializable`.
* Also, the xstream serializer has some special hooks that replaces an entity reference with
- * a marker for that entity, etc. Suggest we change the default {@link #rebind()} to use
- * {@code checkSerializable==false}, and deprecate this + the other overloaded methods?
+ * a marker for that entity, etc.
+ *
+ * @deprecated since 0.7.0; use {@link #rebind()} or {@link #rebind(RebindOptions)})
*/
+ @Deprecated
protected T rebind(boolean checkSerializable) throws Exception {
- // TODO What are sensible defaults?!
- return rebind(checkSerializable, false);
+ return rebind(RebindOptions.create().checkSerializable(checkSerializable));
}
-
- @SuppressWarnings("unchecked")
+
+ /**
+ * Checking serializable is overly strict.
+ * State only needs to be xstream-serializable, which does not require `implements Serializable`.
+ * Also, the xstream serializer has some special hooks that replaces an entity reference with
+ * a marker for that entity, etc.
+ *
+ * @deprecated since 0.7.0; use {@link #rebind(RebindOptions)})
+ */
+ @Deprecated
protected T rebind(boolean checkSerializable, boolean terminateOrigManagementContext) throws Exception {
- return rebind(checkSerializable, terminateOrigManagementContext, (File)null);
+ return rebind(RebindOptions.create()
+ .checkSerializable(checkSerializable)
+ .terminateOrigManagementContext(terminateOrigManagementContext));
}
-
- @Beta // temporary method while debugging; Aled will refactor all of this soon!
- @SuppressWarnings("unchecked")
- protected T rebind(boolean checkSerializable, boolean terminateOrigManagementContext, File backupDir) throws Exception {
- if (newApp!=null || newManagementContext!=null) throw new IllegalStateException("already rebound");
-
- RebindTestUtils.waitForPersisted(origApp);
- if (checkSerializable) {
- RebindTestUtils.checkCurrentMementoSerializable(origApp);
- }
- if (terminateOrigManagementContext) {
- origManagementContext.terminate();
- }
-
- if (backupDir != null) {
- FileUtil.copyDir(mementoDir, backupDir);
- FileUtil.setFilePermissionsTo700(backupDir);
- }
- newManagementContext = createNewManagementContext();
- newApp = (T) RebindTestUtils.rebind((LocalManagementContext)newManagementContext, classLoader);
- return newApp;
+ protected T rebind(RebindExceptionHandler exceptionHandler) throws Exception {
+ return rebind(RebindOptions.create().exceptionHandler(exceptionHandler));
}
+ protected T rebind(ManagementContext newManagementContext, RebindExceptionHandler exceptionHandler) throws Exception {
+ return rebind(RebindOptions.create()
+ .newManagementContext(newManagementContext)
+ .exceptionHandler(exceptionHandler));
+ }
+
@SuppressWarnings("unchecked")
- protected T rebind(RebindExceptionHandler exceptionHandler) throws Exception {
+ protected T rebind(RebindOptions options) throws Exception {
+ if (newApp!=null || newManagementContext!=null) {
+ throw new IllegalStateException("already rebound");
+ }
--- End diff --
Whitespace around binary operators, please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Refactor rebind code, and tidy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/309#issuecomment-62391464
@alasdairhodge code is rebased against master (which now includes #308), so is ready for review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---