You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2016/11/30 23:56:04 UTC
[GitHub] brooklyn-server pull request #472: Revert PR#339: Replace MultimapSerializer...
GitHub user bostko opened a pull request:
https://github.com/apache/brooklyn-server/pull/472
Revert PR#339: Replace MultimapSerializer with jackson-datatype-guava
Reverts https://github.com/apache/brooklyn-server/pull/339/commits/53778394ac9eee9d62fca9535f3c49b8f54f8adf in order to preserve the output format of `/v1/applications/{applicationId}/descendants/sensor/{sensor}`
The new library uses toString() rather than object representation used for this call.
Added simple test for retrieving map sensor.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/brooklyn-server revert_pr339
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/472.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 #472
----
commit 65bce38aba033d04e577e60be9c7af0b127ecb0b
Author: Valentin Aitken <bo...@gmail.com>
Date: 2016-11-30T23:45:11Z
Revert PR#339: Replace MultimapSerializer with jackson-datatype-guava
- add simple test for retrieving map sensor
----
---
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] brooklyn-server issue #472: Revert 5377839: Replace MultimapSerializer with ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/472
Thanks @bostko. I'll wait for tests to pass and will merge it then.
---
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] brooklyn-server pull request #472: Revert 5377839: Replace MultimapSerialize...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/472#discussion_r90421382
--- Diff: rest/rest-server/src/test/java/org/apache/brooklyn/rest/entity/SensorsApiTest.java ---
@@ -0,0 +1,64 @@
+/*
+ * 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.rest.entity;
+
+import com.google.common.collect.Iterables;
+import org.apache.brooklyn.api.entity.EntitySpec;
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.core.entity.Attributes;
+import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests;
+import org.apache.brooklyn.core.test.entity.TestApplication;
+import org.apache.brooklyn.core.test.entity.TestEntity;
+import org.apache.brooklyn.rest.BrooklynRestApiLauncherTestFixture;
+import org.apache.brooklyn.util.net.UserAndHostAndPort;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+public class SensorsApiTest extends BrooklynRestApiLauncherTestFixture {
+ protected ManagementContext mgmt;
+ protected TestApplication app;
+ protected TestEntity entity;
+
+ @BeforeMethod(alwaysRun=true)
+ public void setUp() throws Exception {
+ mgmt = LocalManagementContextForTests.builder(false).build();
+ app = mgmt.getEntityManager().createEntity(EntitySpec.create(TestApplication.class)
+ .child(EntitySpec.create(TestEntity.class)));
+ entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
+
+ useServerForTest(baseLauncher()
+ .managementContext(mgmt)
+ .forceUseOfDefaultCatalogWithJavaClassPath(true)
+ .start());
+ }
+
+ // TODO turn into unit tests, not spinning up real http server.
+ @Test(groups = "Integration")
+ public void testGetHostAndPortSensor() throws Exception {
+ entity.sensors().set(Attributes.SSH_ADDRESS, UserAndHostAndPort.fromParts("testHostUser", "1.2.3.4", 22));
+
+ String sensorGetPath = "/v1/applications/"+app.getId()+"/entities/"+entity.getId()+"/sensors/" + Attributes.SSH_ADDRESS.getName();
+ assertEquals(httpGet(sensorGetPath), "\"testHostUser@1.2.3.4:22\"");
+
+ String descendantSensorPath = "/v1/applications/"+app.getId()+"/descendants/sensor/" + Attributes.SSH_ADDRESS.getName();
+ assertEquals(httpGet(descendantSensorPath), "{\"" + entity.getId() + "\":{\"user\":\"testHostUser\",\"hostAndPort\":{\"host\":\"1.2.3.4\",\"port\":22,\"hasBracketlessColons\":false}}}");
+ }
--- End diff --
Could follow the [`testMultiMapSerialization`](https://github.com/apache/brooklyn-server/blob/master/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java#L195-L203) example how to structure this as a unit test. Agree no need to start an http server.
---
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] brooklyn-server issue #472: Revert 5377839: Replace MultimapSerializer with ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/472
@bostko is there a reason you didn't `git revert` this? (looks like the commit is not a strict reversion as it adds new tests.)
@neykov good summary of the problems. agree having the same guava version is an obvious improvement, but sounds like there will continue to be issues afterwards.
unless there is an easy fix where we can continue to deserialize the old format whilst also being able to serialize and deserialize the new format (which sounds like a lot of work, unless the `GuavaModule` supports it as an option), i'd be inclined _not_ to try to get this working and focus instead on restricting serialization/deserialization to explicitly registered types, ideally using yoml, and defining our own YOML serialization rules for those cases where we really need to serialize a guava type.
---
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] brooklyn-server issue #472: Revert 5377839: Replace MultimapSerializer with ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/472
LGTM. Suggest rewriting the test as commented above. Then good to merge.
I think this is good as a short term fix. Longer term would be nice to get the package back in, resolving the problems it has:
* Changes in `HostAndPort` serialization. We can either accept the new format (warning users beforehand) or register a custom serializer which overrides the `toString` behaviour.
* Binds to guava 18 (used by swagger) thus not able to work with guava 16 objects (used by Brooklyn). I believe we can upgrade our guava version after migrating to jclouds 1.9.3 which properly supports guava 16-20 version ranges.
---
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] brooklyn-server issue #472: Revert 5377839: Replace MultimapSerializer with ...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/472
@ahgittin you are right, sorry. Changed the PR to be with `git revert`.
---
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] brooklyn-server pull request #472: Revert 5377839: Replace MultimapSerialize...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/472#discussion_r90429269
--- Diff: rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java ---
@@ -203,6 +204,14 @@ public void testMultiMapSerialization() throws Exception {
}
@Test
+ public void testUserHostAndPortSerialization() throws Exception {
--- End diff --
@neykov suggested to add that test as well.
Thanks.
---
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] brooklyn-server issue #472: Revert 5377839: Replace MultimapSerializer with ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/472
Test failure seems unrelated:
```
CleanOrphanedLocationsIntegrationTest.testCleanedCopiedPersistedState:167
CleanOrphanedLocationsIntegrationTest.testSelectionWithOrphanedLocationsInData:136->AbstractCleanOrphanedStateTest.assertTransformDeletes:91->AbstractCleanOrphanedStateTest.assertTransformDeletes:101->AbstractCleanOrphanedStateTest.assertRawData:139 expected [true] but found [false]
```
Tests are passing locally. 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] brooklyn-server pull request #472: Revert 5377839: Replace MultimapSerialize...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/472
---
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.
---