DonalEvans commented on a change in pull request #6495:
URL: https://github.com/apache/geode/pull/6495#discussion_r635663946
##########
File path:
geode-core/src/main/java/org/apache/geode/management/internal/configuration/domain/Configuration.java
##########
@@ -183,20 +185,48 @@ public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
propertiesFileName = DataSerializer.readString(in);
gemfireProperties = DataSerializer.readProperties(in);
HashSet<String> jarNames = DataSerializer.readHashSet(in);
- if (jarNames != null) {
- // we are reading pre 1.12 data. So add each jar name to deployments
- jarNames.stream()
- .map(x -> new Deployment(x, null, null))
- .forEach(deployment ->
deployments.put(deployment.getDeploymentName(), deployment));
- } else {
+ try {
// version of the data we are reading (1.12 or later)
final Version version =
Versioning.getVersion(VersioningIO.readOrdinal(in));
if (version.isNotOlderThan(KnownVersion.GEODE_1_12_0)) {
deployments.putAll(DataSerializer.readHashMap(in));
}
+ } catch (EOFException ex) {
+ if (jarNames != null) {
+ // we are reading pre 1.12 data. So add each jar name to deployments
+ jarNames.stream()
+ .map(x -> new Deployment(x, null, null))
+ .forEach(deployment ->
deployments.put(deployment.getDeploymentName(), deployment));
+ }
}
}
+ public void toDataPre_GEODE_1_12_0_0(DataOutput out) throws IOException {
Review comment:
Would it be possible to first call this method in `toData()` and then
add the two extra lines that have been added there, to keep the two methods
consistent?
##########
File path:
geode-core/src/main/java/org/apache/geode/management/internal/configuration/domain/Configuration.java
##########
@@ -183,20 +185,48 @@ public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
propertiesFileName = DataSerializer.readString(in);
gemfireProperties = DataSerializer.readProperties(in);
HashSet<String> jarNames = DataSerializer.readHashSet(in);
- if (jarNames != null) {
- // we are reading pre 1.12 data. So add each jar name to deployments
- jarNames.stream()
- .map(x -> new Deployment(x, null, null))
- .forEach(deployment ->
deployments.put(deployment.getDeploymentName(), deployment));
- } else {
+ try {
Review comment:
Given that we now have a fromDataPre_X method, it should be unnecessary
to have this try/catch block, because we can be sure that we'll be reading from
a member with the correct version here.
##########
File path:
geode-core/src/main/java/org/apache/geode/management/internal/configuration/domain/Configuration.java
##########
@@ -183,20 +185,48 @@ public void fromData(DataInput in) throws IOException,
ClassNotFoundException {
propertiesFileName = DataSerializer.readString(in);
gemfireProperties = DataSerializer.readProperties(in);
HashSet<String> jarNames = DataSerializer.readHashSet(in);
- if (jarNames != null) {
- // we are reading pre 1.12 data. So add each jar name to deployments
- jarNames.stream()
- .map(x -> new Deployment(x, null, null))
- .forEach(deployment ->
deployments.put(deployment.getDeploymentName(), deployment));
- } else {
+ try {
// version of the data we are reading (1.12 or later)
final Version version =
Versioning.getVersion(VersioningIO.readOrdinal(in));
if (version.isNotOlderThan(KnownVersion.GEODE_1_12_0)) {
deployments.putAll(DataSerializer.readHashMap(in));
}
+ } catch (EOFException ex) {
+ if (jarNames != null) {
+ // we are reading pre 1.12 data. So add each jar name to deployments
+ jarNames.stream()
+ .map(x -> new Deployment(x, null, null))
+ .forEach(deployment ->
deployments.put(deployment.getDeploymentName(), deployment));
+ }
}
}
+ public void toDataPre_GEODE_1_12_0_0(DataOutput out) throws IOException {
+ DataSerializer.writeString(configName, out);
+ DataSerializer.writeString(cacheXmlFileName, out);
+ DataSerializer.writeString(cacheXmlContent, out);
+ DataSerializer.writeString(propertiesFileName, out);
+ DataSerializer.writeProperties(gemfireProperties, out);
+ HashSet<String> jarNames = new HashSet<>(deployments.keySet());
+ DataSerializer.writeHashSet(jarNames, out);
+ }
+
+ public void fromDataPre_GEODE_1_12_0_0(DataInput in) throws IOException,
ClassNotFoundException {
Review comment:
I know that the implementations differ when it comes to how to handle
the `jarNames` set, but would it be possible to find a way to have `fromData()`
first call this method and then do any additional work that's specific to the
post-1.12.0 implementation, just to reduce code duplication?
##########
File path:
geode-gfsh/src/upgradeTest/java/org/apache/geode/management/ConfigurationCompatibilityTest.java
##########
@@ -0,0 +1,109 @@
+/*
+ * 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.geode.management;
+
+
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.geode.cache.RegionShortcut;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.categories.BackwardCompatibilityTest;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+import org.apache.geode.test.version.TestVersion;
+import org.apache.geode.test.version.VersionManager;
+
+@Category({BackwardCompatibilityTest.class})
+@RunWith(Parameterized.class)
+public class ConfigurationCompatibilityTest {
+ private final String oldVersion;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static Collection<String> data() {
+ List<String> result =
VersionManager.getInstance().getVersionsWithoutCurrent();
+ result.removeIf(s -> TestVersion.compare(s, "1.10.0") < 0);
+ return result;
+ }
+
+ public ConfigurationCompatibilityTest(String oldVersion) {
+ this.oldVersion = oldVersion;
+ }
+
+ @Rule
+ public ClusterStartupRule clusterStartupRule = new ClusterStartupRule();
+
+ @Rule
+ public GfshCommandRule gfsh = new GfshCommandRule();
+
+ @Test
+ public void
whenConfigurationIsExchangedBetweenMixedVersionLocatorsThenItShouldNotThrowExceptions()
Review comment:
When I revert the changes in this PR, this test still passes, so I'm not
sure that it's testing what you're intending it to test.
--
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:
[email protected]
