[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396484281
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
##
@@ -556,6 +553,11 @@ public void startMonitor() throws Exception {
log.remoteConfigurationMonitorStartFailure(remoteMonitor.getClass().getTypeName(),
e.getLocalizedMessage());
}
}
+
+// Trigger descriptor discovery (KNOX-2301)
+for (File descriptor : getDescriptors()) {
+ descriptor.setLastModified(System.currentTimeMillis()); // 'Touch' the
descriptor
+}
Review comment:
This is the reason for the comment, but pulling it out into a separate
explicitly-named method is more clear and resilient to future confusion from
the comment not being updated.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396482734
##
File path:
gateway-topology-simple/src/main/java/org/apache/knox/gateway/topology/simple/SimpleDescriptorHandler.java
##
@@ -571,13 +579,49 @@ private static File
resolveProviderConfigurationReference(final String reference
if (topologyFilename == null) {
topologyFilename = desc.getCluster();
}
+
topologyDescriptor = new File(destDirectory, topologyFilename +
".xml");
-try (OutputStream outputStream =
Files.newOutputStream(topologyDescriptor.toPath());
- OutputStreamWriter outputStreamWriter = new
OutputStreamWriter(outputStream, StandardCharsets.UTF_8);
- BufferedWriter fw = new BufferedWriter(outputStreamWriter)) {
- fw.write(sw.toString());
- fw.flush();
+// KNOX-2302
+boolean persistGenerated = false;
+// Determine if the topology already exists.
+if (topologyDescriptor.exists()) {
Review comment:
+1
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396475018
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/topology/Service.java
##
@@ -122,32 +122,61 @@ public boolean equals(Object object) {
return false;
}
Service that = (Service) object;
-String thatName = that.getName();
+String thatName = that.name;
if (thatName != null && !(thatName.equals(name))) {
return false;
}
-String thatRole = that.getRole();
+String thatRole = that.role;
if (thatRole != null && !thatRole.equals(role)) {
return false;
}
-Version thatVersion = that.getVersion();
+Version thatVersion = that.version;
if (thatVersion != null && !(thatVersion.equals(version))) {
return false;
}
+
+// URLs
+if (urls.size() != that.urls.size()) {
+ return false;
+}
+
+for (String url : urls) {
+ if (!that.urls.contains(url)) {
+return false;
+ }
+}
+
+// Params
+if (params.size() != that.params.size()) {
+ return false;
+}
+
+for (Map.Entry param : params.entrySet()) {
+ if (!param.getValue().equals(that.params.get(param.getKey( {
+return false;
+ }
+}
+
return true;
}
@Override
public int hashCode() {
Review comment:
+1
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396475589
##
File path:
gateway-topology-simple/src/main/java/org/apache/knox/gateway/topology/simple/SimpleDescriptorHandler.java
##
@@ -571,13 +579,49 @@ private static File
resolveProviderConfigurationReference(final String reference
if (topologyFilename == null) {
topologyFilename = desc.getCluster();
}
+
topologyDescriptor = new File(destDirectory, topologyFilename +
".xml");
-try (OutputStream outputStream =
Files.newOutputStream(topologyDescriptor.toPath());
- OutputStreamWriter outputStreamWriter = new
OutputStreamWriter(outputStream, StandardCharsets.UTF_8);
- BufferedWriter fw = new BufferedWriter(outputStreamWriter)) {
- fw.write(sw.toString());
- fw.flush();
+// KNOX-2302
+boolean persistGenerated = false;
+// Determine if the topology already exists.
+if (topologyDescriptor.exists()) {
+// If it does, only overwrite it if it has changed (KNOX-2302)
+// Compare the generated topology with the in-memory topology
+Topology existing = null;
+TopologyService topologyService =
gwServices.getService(ServiceType.TOPOLOGY_SERVICE);
+for (Topology t : topologyService.getTopologies()) {
+if (topologyFilename.equals(t.getName())) {
+existing = t;
+break;
+}
Review comment:
I feel this is out of scope. It is not required for this PR.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396466990
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/topology/Provider.java
##
@@ -82,4 +84,41 @@ public void setRole( String role ) {
this.role = role;
}
+ @Override
+ public int hashCode() {
+return new HashCodeBuilder().append(name)
+.append(role)
+.append(params)
+.append(enabled)
+.build();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
Review comment:
+1
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396466614
##
File path:
gateway-test/src/test/java/org/apache/knox/gateway/AmbariServiceDefinitionTest.java
##
@@ -114,6 +114,12 @@ public static void setupGateway() throws Exception {
File deployDir = new File( config.getGatewayDeploymentDir() );
deployDir.mkdirs();
+File descDir = new File( config.getGatewayDescriptorsDir() );
Review comment:
I had to make these changes to get the tests to pass. It is a side-effect
requirement of the changes in this PR.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396466116
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/topology/Topology.java
##
@@ -153,4 +161,106 @@ public void addProvider( Provider provider ) {
nameMap.put( provider.getName(), provider );
}
+ @Override
+ public int hashCode() {
+return (new HashCodeBuilder(17, 31)).append(name)
+.append(providerList)
+.append(services)
+.append(applications)
+.build();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
Review comment:
Not using EqualsBuilder here because it does not handle potentially-null
fields well.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396455084
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/topology/Topology.java
##
@@ -153,4 +161,106 @@ public void addProvider( Provider provider ) {
nameMap.put( provider.getName(), provider );
}
+ @Override
+ public int hashCode() {
+return (new HashCodeBuilder(17, 31)).append(name)
+.append(providerList)
+.append(services)
+.append(applications)
+.build();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
Review comment:
The reason for not using EqualsBuilder here is that equals() for Lists
requires the ordering to be the same. However, for the purpose of evaluating
topologies for equality, such ordering is not important.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396455084
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/topology/Topology.java
##
@@ -153,4 +161,106 @@ public void addProvider( Provider provider ) {
nameMap.put( provider.getName(), provider );
}
+ @Override
+ public int hashCode() {
+return (new HashCodeBuilder(17, 31)).append(name)
+.append(providerList)
+.append(services)
+.append(applications)
+.build();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
Review comment:
The reason for not using EqualsBuilder here is that equals() for Lists
requires the ordering to be the same. However, for the purpose of evaluating
topologies for equality, such ordering is not important.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396452322
##
File path:
gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java
##
@@ -177,6 +181,17 @@ public void testGetTopologies() throws Exception {
}
}
+ /**
+ * Set the static GatewayServices field to the specified value.
+ *
+ * @param gws A GatewayServices object, or null.
+ */
+ private void setGatewayServices(final GatewayServices gws) throws Exception {
+Field gwsField = GatewayServer.class.getDeclaredField("services");
+gwsField.setAccessible(true);
+gwsField.set(null, gws);
+ }
+
Review comment:
Your understanding is correct.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396029708
##
File path:
gateway-topology-simple/src/main/java/org/apache/knox/gateway/topology/simple/TopologyComparisonUtil.java
##
@@ -0,0 +1,192 @@
+/*
+ * 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.knox.gateway.topology.simple;
+
+import org.apache.knox.gateway.topology.Application;
+import org.apache.knox.gateway.topology.Provider;
+import org.apache.knox.gateway.topology.Service;
+import org.apache.knox.gateway.topology.Topology;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Utility for comparing Topology objects for equivalence.
+ */
+class TopologyComparisonUtil {
Review comment:
Moved to Topology#equals() method.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395996190
##
File path:
gateway-topology-simple/src/main/java/org/apache/knox/gateway/topology/simple/TopologyComparisonUtil.java
##
@@ -0,0 +1,192 @@
+/*
+ * 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.knox.gateway.topology.simple;
+
+import org.apache.knox.gateway.topology.Application;
+import org.apache.knox.gateway.topology.Provider;
+import org.apache.knox.gateway.topology.Service;
+import org.apache.knox.gateway.topology.Topology;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Utility for comparing Topology objects for equivalence.
+ */
+class TopologyComparisonUtil {
Review comment:
I'm already converting JSON to a Topology object for this purpose, and would
prefer to avoid re-converting them back to JSON for the sake of comparison.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395996016
##
File path:
gateway-topology-simple/src/main/java/org/apache/knox/gateway/topology/simple/TopologyComparisonUtil.java
##
@@ -0,0 +1,192 @@
+/*
+ * 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.knox.gateway.topology.simple;
+
+import org.apache.knox.gateway.topology.Application;
+import org.apache.knox.gateway.topology.Provider;
+import org.apache.knox.gateway.topology.Service;
+import org.apache.knox.gateway.topology.Topology;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * Utility for comparing Topology objects for equivalence.
+ */
+class TopologyComparisonUtil {
Review comment:
This thought had crossed my mind as I was implementing it; I think the fact
that this requirement doesn't care about the URI, default service path or
timestamp is what made me opt for this. I think a complete equals()
implementation would include at least the URI, but if others disagree, I could
be persuaded to move this logic into the Topology equals() method.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395995104
##
File path:
gateway-server/src/test/java/org/apache/knox/gateway/services/topology/DefaultTopologyServiceTest.java
##
@@ -177,6 +181,21 @@ public void testGetTopologies() throws Exception {
}
}
+ /**
+ * Set the static GatewayServices field to the specified value.
+ *
+ * @param gws A GatewayServices object, or null.
+ */
+ private void setGatewayServices(final GatewayServices gws) {
+try {
+ Field gwsField = GatewayServer.class.getDeclaredField("services");
+ gwsField.setAccessible(true);
+ gwsField.set(null, gws);
+} catch (Exception e) {
+ e.printStackTrace();
Review comment:
+1
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395995034
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
##
@@ -153,17 +146,18 @@ private Topology loadTopology(File file) throws
IOException, SAXException, Inter
return topology;
}
+ @Override
+ public Topology parse(final InputStream content) throws IOException,
SAXException {
+return TopologyUtils.parse(content);
+ }
+
private Topology loadTopologyAttempt(File file) throws IOException,
SAXException {
-Topology topology;
-Digester digester = digesterLoader.newDigester();
-TopologyBuilder topologyBuilder =
digester.parse(FileUtils.openInputStream(file));
-if (null == topologyBuilder) {
- return null;
-}
-topology = topologyBuilder.build();
-topology.setUri(file.toURI());
-topology.setName(FilenameUtils.removeExtension(file.getName()));
-topology.setTimestamp(file.lastModified());
+Topology topology = parse(FileUtils.openInputStream(file));
Review comment:
The reason for the TopologyService method taking an InputStream (rather than
a File) is that there is not a File available in every case. In one case, it is
being used to parse a generated String. There could be two signatures, but I
think the InputStream signature supports the two existing cases well enough.
The streams must be closed though.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
pzampino commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395688935
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java
##
@@ -153,17 +146,18 @@ private Topology loadTopology(File file) throws
IOException, SAXException, Inter
return topology;
}
+ @Override
+ public Topology parse(final InputStream content) throws IOException,
SAXException {
+return TopologyUtils.parse(content);
+ }
+
private Topology loadTopologyAttempt(File file) throws IOException,
SAXException {
-Topology topology;
-Digester digester = digesterLoader.newDigester();
-TopologyBuilder topologyBuilder =
digester.parse(FileUtils.openInputStream(file));
-if (null == topologyBuilder) {
- return null;
-}
-topology = topologyBuilder.build();
-topology.setUri(file.toURI());
-topology.setName(FilenameUtils.removeExtension(file.getName()));
-topology.setTimestamp(file.lastModified());
+Topology topology = parse(FileUtils.openInputStream(file));
Review comment:
That makes sense.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
