You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/03/20 03:23:20 UTC
[GitHub] [knox] pzampino opened a new pull request #297: KNOX-2301 and
KNOX-2302
pzampino opened a new pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297
## What changes were proposed in this pull request?
These are two closely-related issues. Added (actually, removed restrictions on) performing re-discovery for every applicable descriptor on gateway start-up. To prevent unnecessary redeployments of unchanged topologies, also added a check to skip the persistence of generated topologies if there exists an equivalent deployed topology of the same name.
## How was this patch tested?
A great deal of manual testing was performed, and a number of existing tests were modified. Also added some new tests.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396332466
##########
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:
I'd move this out to a new method instead of keeping it in `startMonitor` with a good method name. Otherwise, in the future, it'd confuse others why it's here.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395508911
##########
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));
+ if (topology != null) {
+ topology.setUri(file.toURI());
+ topology.setName(FilenameUtils.removeExtension(file.getName()));
+ topology.setTimestamp(file.lastModified());
+ }
Review comment:
If we pass in the file reference, you can move this logic in there too and get the full topology object back.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino merged pull request #297: KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396337421
##########
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:
If I understand well this is implemented because EasyMock does not support static method mocking. I'm ok with this change, however, you may consider using [PowerMock](https://github.com/powermock/powermock)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396352071
##########
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:
I'd move this new logic to its own private method with a meaningful name (e.g. `shouldRedeployTopology`) to make the code cleaner.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396341268
##########
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<String, String> param : params.entrySet()) {
+ if (!param.getValue().equals(that.params.get(param.getKey()))) {
+ return false;
+ }
+ }
+
return true;
}
@Override
public int hashCode() {
Review comment:
Adding `HashCodeBuilder` and `EqualsBuilder` here too?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395508508
##########
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:
I think it would be cleaner to open/close input stream in the parse method. Basically pass a file reference and not an input stream?
Otherwise I think we need to make sure to close the input stream?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396351060
##########
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:
Maybe introducing `topologyService.getTopology(String topologyName)`?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396347775
##########
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:
This is out-of-scope of this JIRA so I filed a new one: https://issues.apache.org/jira/browse/KNOX-2309
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395511204
##########
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:
Is this class necessary? Can't we just ask if the two topology objects are equal? Most of this logic looks like it should be in an 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396338380
##########
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:
I really like you are using `HashCodeBuilder` above. Why not use `EqualsBuilder` here (this would require to implement `hashCode` and `equals` in `Provider` too)?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395711086
##########
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:
Assuming the JSON representation of `topology1` and `topology2` are the same in case they are equal you can simply do a test like:
`org.apache.knox.gateway.util.JsonUtils.renderAsJsonString(topology1).equals(JsonUtils.renderAsJsonString(topology2)`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r395509835
##########
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:
Not sure you need the catch here? Since it's a test probably better to throw it?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396333267
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/util/TopologyUtils.java
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.util;
+
+import org.apache.commons.digester3.binder.DigesterLoader;
+import org.apache.knox.gateway.topology.Topology;
+import org.apache.knox.gateway.topology.builder.TopologyBuilder;
+import org.apache.knox.gateway.topology.xml.AmbariFormatXmlTopologyRules;
+import org.apache.knox.gateway.topology.xml.KnoxFormatXmlTopologyRules;
+import org.xml.sax.SAXException;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.StringReader;
+
+import static org.apache.commons.digester3.binder.DigesterLoader.newLoader;
+
+public class TopologyUtils {
+
+ private static DigesterLoader digesterLoader = newLoader(new KnoxFormatXmlTopologyRules(),
Review comment:
nit: can be `final`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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<String, String> 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] smolnar82 commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #297: KNOX-2301 and KNOX-2302
URL: https://github.com/apache/knox/pull/297#discussion_r396342594
##########
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:
If `Provider`, `Service` and `Application` implements `hashCode` and `equals` properly you do not need the private methods below and you can simply use `EqualsBuilder` here too.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #297:
KNOX-2301 and KNOX-2302
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services