pivotal-jbarrett commented on a change in pull request #5905:
URL: https://github.com/apache/geode/pull/5905#discussion_r561402332
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/BackwardCompatibilitySerializationDUnitTest.java
##########
@@ -59,11 +56,11 @@
private transient ByteArrayInputStream bais;
public static boolean toDataCalled = false;
- public static boolean toDataPre66Called = false;
- public static boolean toDataPre70called = false;
+ public static boolean toDataPre11Called = false;
+ public static boolean toDataPre15called = false;
public static boolean fromDataCalled = false;
- public static boolean fromDataPre66Called = false;
- public static boolean fromDataPre70Called = false;
+ public static boolean fromDataPre11Called = false;
+ public static boolean fromDataPre15Called = false;
Review comment:
Yup, totally arbitrary really.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/query/internal/cq/CqServiceProvider.java
##########
@@ -59,7 +60,7 @@ public static CqService create(InternalCache cache) {
return new MissingCqService();
}
- return factory.create(cache);
+ return factory.create(cache, CommandInitializer.getDefaultInstance());
Review comment:
Not without a much deeper refactoring. We will need a way to connect two
very distant components and that is just out of scope for what I want to get
done now. So it's no worse than it was. It should discourage continued abuse.
It is now testable. So I see a net positive here.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CommandInitializer.java
##########
@@ -111,37 +89,76 @@
* @since GemFire 5.7
*/
-public class CommandInitializer {
+public class CommandInitializer implements CommandRegistry {
- @Immutable
- static final Map<KnownVersion, Map<Integer, Command>> ALL_COMMANDS =
initializeAllCommands();
+ @Deprecated
+ @MakeNotStatic
+ static final CommandInitializer instance = new CommandInitializer();
/**
- * Register a new command with the system.
+ * Gets legacy singleton instance.
+ *
+ * @deprecated Efforts should be made to get and instance from the cache or
other object.
*
- * @param messageType - An ordinal for this message. This must be something
defined in MessageType
- * that has not already been allocated to a different command.
- * @param versionToNewCommand The command to register, for different
versions. The key is the
- * earliest version for which this command class is valid (starting
with GFE_57). The value
- * is the command object for clients starting with that version.
+ * @return legacy singleton instance. Instance is not immutable.
*/
- public static void registerCommand(int messageType,
+ @Deprecated
+ public static CommandInitializer getDefaultInstance() {
+ return instance;
+ }
+
+ final Map<KnownVersion, Map<Integer, Command>>
unmodifiableRegisteredCommands;
+ final LinkedHashMap<KnownVersion, ConcurrentMap<Integer, Command>>
modifiableRegisteredCommands;
Review comment:
Ah... to maintain the contract that the maps returned by the
`get(Version)` method are immutable views. The previous implementation made the
outer map immutable but not the inner map so the caller could technically
mutate the map causing who knows what issues. Now the only way to mutate is
through `register`, which mutates the backing maps but keeps the immutable
wrappers intact.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CommandInitializer.java
##########
@@ -111,37 +89,76 @@
* @since GemFire 5.7
*/
-public class CommandInitializer {
+public class CommandInitializer implements CommandRegistry {
- @Immutable
- static final Map<KnownVersion, Map<Integer, Command>> ALL_COMMANDS =
initializeAllCommands();
+ @Deprecated
+ @MakeNotStatic
+ static final CommandInitializer instance = new CommandInitializer();
/**
- * Register a new command with the system.
+ * Gets legacy singleton instance.
+ *
+ * @deprecated Efforts should be made to get and instance from the cache or
other object.
*
- * @param messageType - An ordinal for this message. This must be something
defined in MessageType
- * that has not already been allocated to a different command.
- * @param versionToNewCommand The command to register, for different
versions. The key is the
- * earliest version for which this command class is valid (starting
with GFE_57). The value
- * is the command object for clients starting with that version.
+ * @return legacy singleton instance. Instance is not immutable.
*/
- public static void registerCommand(int messageType,
+ @Deprecated
+ public static CommandInitializer getDefaultInstance() {
+ return instance;
+ }
+
+ final Map<KnownVersion, Map<Integer, Command>>
unmodifiableRegisteredCommands;
+ final LinkedHashMap<KnownVersion, ConcurrentMap<Integer, Command>>
modifiableRegisteredCommands;
Review comment:
You can think of the unmodifiable version of the map as just a map of
unmodifiable wrappers. It doesn't contain any data itself. I am not committed
to it though if you don't like it.
##########
File path:
geode-serialization/src/main/java/org/apache/geode/internal/serialization/KnownVersion.java
##########
@@ -367,61 +377,48 @@ public static String unsupportedVersionMessage(final
short ordinal) {
}
public String getMethodSuffix() {
- return this.methodSuffix;
+ return methodSuffix;
}
public String getName() {
- return this.name;
+ return name;
}
- public short getMajorVersion() {
- return this.majorVersion;
+ public short getMajor() {
+ return major;
}
- public short getMinorVersion() {
- return this.minorVersion;
+ public short getMinor() {
+ return minor;
}
public short getRelease() {
- return this.release;
+ return release;
}
public short getPatch() {
- return this.patch;
+ return patch;
}
- /**
- * Returns a string representation for this <code>Version</code>.
- *
- * @return the name of this operation.
- */
@Override
public String toString() {
- return this.productName + " " + this.name;
- }
-
- public byte[] toBytes() {
- byte[] bytes = new byte[2];
- bytes[0] = (byte) (ordinal() >> 8);
- bytes[1] = (byte) ordinal();
- return bytes;
+ return productName + " " + name;
}
public static Iterable<? extends KnownVersion> getAllVersions() {
- return Arrays.asList(VALUES).stream().filter(x -> x != null && x !=
TEST_VERSION)
+ return Arrays.stream(VALUES).filter(x -> x != null && x != TEST_VERSION)
.collect(Collectors.toList());
}
/**
* package-protected for use by Versioning factory
*/
- static KnownVersion getKnownVersionOrDefault(final short ordinal,
- final KnownVersion defaultKnownVersion) {
+ static KnownVersion getKnownVersionOrDefault(final short ordinal) {
Review comment:
It turns out it is only used in one place too. I think I am just going
to remove it.
##########
File path:
geode-serialization/src/main/java/org/apache/geode/internal/serialization/KnownVersion.java
##########
@@ -367,61 +377,48 @@ public static String unsupportedVersionMessage(final
short ordinal) {
}
public String getMethodSuffix() {
- return this.methodSuffix;
+ return methodSuffix;
}
public String getName() {
- return this.name;
+ return name;
}
- public short getMajorVersion() {
- return this.majorVersion;
+ public short getMajor() {
+ return major;
}
- public short getMinorVersion() {
- return this.minorVersion;
+ public short getMinor() {
+ return minor;
}
public short getRelease() {
- return this.release;
+ return release;
}
public short getPatch() {
- return this.patch;
+ return patch;
}
- /**
- * Returns a string representation for this <code>Version</code>.
- *
- * @return the name of this operation.
- */
@Override
public String toString() {
- return this.productName + " " + this.name;
- }
-
- public byte[] toBytes() {
- byte[] bytes = new byte[2];
- bytes[0] = (byte) (ordinal() >> 8);
- bytes[1] = (byte) ordinal();
- return bytes;
+ return productName + " " + name;
}
public static Iterable<? extends KnownVersion> getAllVersions() {
- return Arrays.asList(VALUES).stream().filter(x -> x != null && x !=
TEST_VERSION)
+ return Arrays.stream(VALUES).filter(x -> x != null && x != TEST_VERSION)
.collect(Collectors.toList());
}
/**
* package-protected for use by Versioning factory
*/
- static KnownVersion getKnownVersionOrDefault(final short ordinal,
- final KnownVersion defaultKnownVersion) {
+ static KnownVersion getKnownVersionOrDefault(final short ordinal) {
Review comment:
I take that back, I won't remove it since it encapsulates some
nastiness. But I will rename it.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/BackwardCompatibilitySerializationDUnitTest.java
##########
@@ -160,7 +149,7 @@ public void testFromDataFromLowerVersionToHigher() throws
Exception {
public void testAllMessages() throws Exception {
// list of msgs not created using reflection
// taken from DSFIDFactory.create()
- ArrayList<Integer> constdsfids = new ArrayList<Integer>();
+ ArrayList<Integer> constdsfids = new ArrayList<>();
Review comment:
Do you think this test is supposed to be testing all those types as
well? I think that might be out of scope for this PR. My gut tells me this test
was testing the idea of backwards compatibility and not the specifics of
messages.
##########
File path:
geode-core/src/integrationTest/java/org/apache/geode/cache/client/ClientCacheFactoryJUnitTest.java
##########
@@ -367,13 +366,13 @@ public void testOldClientIDDeserialization() throws
Exception {
ClientProxyMembershipID newID = DataSerializer.readObject(in);
InternalDistributedMember newMemberID =
(InternalDistributedMember) newID.getDistributedMember();
- assertThat(newMemberID.getVersion()).isEqualTo(KnownVersion.GFE_82);
- assertThat(newID.getClientVersion()).isEqualTo(KnownVersion.GFE_82);
+ assertThat(newMemberID.getVersion()).isEqualTo(KnownVersion.GFE_81);
+ assertThat(newID.getClientVersion()).isEqualTo(KnownVersion.GFE_81);
assertThat(newMemberID.getUuidLeastSignificantBits()).isEqualTo(0);
assertThat(newMemberID.getUuidMostSignificantBits()).isEqualTo(0);
- gmsID.setUUID(new UUID(1234L, 5678L));
+ ((MemberIdentifier) memberID).setUUID(new UUID(1234L, 5678L));
Review comment:
Very odd. I wonder what I did for it to add those casts. Fixing.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientSideHandshakeImpl.java
##########
@@ -233,7 +230,7 @@ public ServerQueueStatus handshakeWithServer(Connection
conn, ServerLocation loc
member = readServerMember(dis);
- serverQStatus = new ServerQueueStatus(endpointType, queueSize, member);
+ ServerQueueStatus serverQStatus = new ServerQueueStatus(endpointType,
queueSize, member);
Review comment:
Could be said for so many places. What made you pick this one place I
missed. ;)
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CommandInitializer.java
##########
@@ -111,37 +89,76 @@
* @since GemFire 5.7
*/
-public class CommandInitializer {
+public class CommandInitializer implements CommandRegistry {
- @Immutable
- static final Map<KnownVersion, Map<Integer, Command>> ALL_COMMANDS =
initializeAllCommands();
+ @Deprecated
+ @MakeNotStatic
+ static final CommandInitializer instance = new CommandInitializer();
/**
- * Register a new command with the system.
+ * Gets legacy singleton instance.
+ *
+ * @deprecated Efforts should be made to get and instance from the cache or
other object.
*
- * @param messageType - An ordinal for this message. This must be something
defined in MessageType
- * that has not already been allocated to a different command.
- * @param versionToNewCommand The command to register, for different
versions. The key is the
- * earliest version for which this command class is valid (starting
with GFE_57). The value
- * is the command object for clients starting with that version.
+ * @return legacy singleton instance. Instance is not immutable.
*/
- public static void registerCommand(int messageType,
+ @Deprecated
+ public static CommandInitializer getDefaultInstance() {
+ return instance;
+ }
+
+ final Map<KnownVersion, Map<Integer, Command>>
unmodifiableRegisteredCommands;
+ final LinkedHashMap<KnownVersion, ConcurrentMap<Integer, Command>>
modifiableRegisteredCommands;
Review comment:
The unmodifiable maps are just wrappers and not copies so changes made
to the modifiable map are visible to unmodifiable map.
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientSideHandshakeImpl.java
##########
@@ -125,23 +123,23 @@ public static void setVersionForTesting(short ver) {
}
private void setOverrides() {
- this.clientConflation = determineClientConflation();
+ clientConflation = determineClientConflation();
// As of May 2009 ( GFE 6.0 ):
// Note that this.clientVersion is used by server side for accepting
// handshakes.
// Client side handshake code uses this.currentClientVersion which can be
// set via tests.
if (currentClientVersion.isNotOlderThan(KnownVersion.GFE_603)) {
Review comment:
Removal of older version checks are largely out of scope for this PR. As
per the proposal email, the goal is to set the ground work for the much larger
effort of removal.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/BaseCommandJUnitTest.java
##########
@@ -27,18 +27,13 @@
import org.junit.runner.RunWith;
import
org.apache.geode.internal.cache.execute.ServerToClientFunctionResultSender;
-import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunction61;
-import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunction65;
import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunction66;
@RunWith(JUnitParamsRunner.class)
public class BaseCommandJUnitTest {
public BaseCommand[] getCommands() {
- return new BaseCommand[] {(BaseCommand)
ExecuteRegionFunction61.getCommand(),
- (BaseCommand) ExecuteRegionFunction65
- .getCommand(),
- (BaseCommand) ExecuteRegionFunction66.getCommand()};
+ return new BaseCommand[] {(BaseCommand)
ExecuteRegionFunction66.getCommand()};
Review comment:
Keen eye!
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CommandInitializerTest.java
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.internal.cache.tier.sockets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.junit.Test;
+
+import org.apache.geode.internal.cache.tier.Command;
+import org.apache.geode.internal.cache.tier.MessageType;
+import org.apache.geode.internal.cache.tier.sockets.command.AddPdxEnum;
+import org.apache.geode.internal.cache.tier.sockets.command.AddPdxType;
+import org.apache.geode.internal.cache.tier.sockets.command.ClearRegion;
+import org.apache.geode.internal.cache.tier.sockets.command.ClientReady;
+import org.apache.geode.internal.cache.tier.sockets.command.CloseConnection;
+import org.apache.geode.internal.cache.tier.sockets.command.CommitCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.ContainsKey66;
+import org.apache.geode.internal.cache.tier.sockets.command.CreateRegion;
+import org.apache.geode.internal.cache.tier.sockets.command.Destroy70;
+import org.apache.geode.internal.cache.tier.sockets.command.DestroyRegion;
+import org.apache.geode.internal.cache.tier.sockets.command.ExecuteFunction70;
+import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunction66;
+import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunctionGeode18;
+import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunctionSingleHop;
+import
org.apache.geode.internal.cache.tier.sockets.command.GatewayReceiverCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.Get70;
+import org.apache.geode.internal.cache.tier.sockets.command.GetAll70;
+import org.apache.geode.internal.cache.tier.sockets.command.GetAllWithCallback;
+import
org.apache.geode.internal.cache.tier.sockets.command.GetClientPRMetadataCommand66;
+import
org.apache.geode.internal.cache.tier.sockets.command.GetClientPartitionAttributesCommand66;
+import org.apache.geode.internal.cache.tier.sockets.command.GetEntry70;
+import
org.apache.geode.internal.cache.tier.sockets.command.GetFunctionAttribute;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXEnumById;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXIdForEnum;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXIdForType;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXTypeById;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPdxEnums70;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPdxTypes70;
+import org.apache.geode.internal.cache.tier.sockets.command.Invalid;
+import org.apache.geode.internal.cache.tier.sockets.command.Invalidate70;
+import org.apache.geode.internal.cache.tier.sockets.command.KeySet;
+import org.apache.geode.internal.cache.tier.sockets.command.MakePrimary;
+import org.apache.geode.internal.cache.tier.sockets.command.PeriodicAck;
+import org.apache.geode.internal.cache.tier.sockets.command.Ping;
+import org.apache.geode.internal.cache.tier.sockets.command.Put70;
+import org.apache.geode.internal.cache.tier.sockets.command.PutAll80;
+import org.apache.geode.internal.cache.tier.sockets.command.PutAllWithCallback;
+import org.apache.geode.internal.cache.tier.sockets.command.PutUserCredentials;
+import org.apache.geode.internal.cache.tier.sockets.command.Query651;
+import org.apache.geode.internal.cache.tier.sockets.command.QueryGeode10;
+import
org.apache.geode.internal.cache.tier.sockets.command.QueryWithParametersGeode10;
+import
org.apache.geode.internal.cache.tier.sockets.command.RegisterDataSerializers;
+import
org.apache.geode.internal.cache.tier.sockets.command.RegisterInstantiators;
+import org.apache.geode.internal.cache.tier.sockets.command.RegisterInterest61;
+import
org.apache.geode.internal.cache.tier.sockets.command.RegisterInterestList66;
+import org.apache.geode.internal.cache.tier.sockets.command.RemoveAll;
+import org.apache.geode.internal.cache.tier.sockets.command.RemoveUserAuth;
+import org.apache.geode.internal.cache.tier.sockets.command.RequestEventValue;
+import org.apache.geode.internal.cache.tier.sockets.command.RollbackCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.Size;
+import org.apache.geode.internal.cache.tier.sockets.command.TXFailoverCommand;
+import
org.apache.geode.internal.cache.tier.sockets.command.TXSynchronizationCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.UnregisterInterest;
+import
org.apache.geode.internal.cache.tier.sockets.command.UnregisterInterestList;
+import
org.apache.geode.internal.cache.tier.sockets.command.UpdateClientNotification;
+import org.apache.geode.internal.serialization.KnownVersion;
+
+public class CommandInitializerTest {
+
+ @Test
+ public void testCommandMapContainsAllVersions() {
+ for (KnownVersion version : KnownVersion.getAllVersions()) {
+ if (version.isNotOlderThan(KnownVersion.OLDEST)) {
+ org.junit.Assert.assertNotNull(
+ "Please add a command set for " + version + " of Geode to
CommandInitializer",
+ CommandInitializer.getDefaultInstance().get(version));
+ }
+ }
+ }
+
+ @Test
+ public void initializeGeode18Commands() {
+ @SuppressWarnings("unchecked")
+ final Map<Integer, Command> commands = mock(Map.class);
Review comment:
Honestly I could delete this test. There is an integration test that
does pretty much what you describe. The rational for this test was to assert
that I didn't have any duplicates when I refactored each of the versions.
Checking the map wouldn't weed out two puts on the same key but this does.
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/CommandInitializerTest.java
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.internal.cache.tier.sockets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.junit.Test;
+
+import org.apache.geode.internal.cache.tier.Command;
+import org.apache.geode.internal.cache.tier.MessageType;
+import org.apache.geode.internal.cache.tier.sockets.command.AddPdxEnum;
+import org.apache.geode.internal.cache.tier.sockets.command.AddPdxType;
+import org.apache.geode.internal.cache.tier.sockets.command.ClearRegion;
+import org.apache.geode.internal.cache.tier.sockets.command.ClientReady;
+import org.apache.geode.internal.cache.tier.sockets.command.CloseConnection;
+import org.apache.geode.internal.cache.tier.sockets.command.CommitCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.ContainsKey66;
+import org.apache.geode.internal.cache.tier.sockets.command.CreateRegion;
+import org.apache.geode.internal.cache.tier.sockets.command.Destroy70;
+import org.apache.geode.internal.cache.tier.sockets.command.DestroyRegion;
+import org.apache.geode.internal.cache.tier.sockets.command.ExecuteFunction70;
+import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunction66;
+import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunctionGeode18;
+import
org.apache.geode.internal.cache.tier.sockets.command.ExecuteRegionFunctionSingleHop;
+import
org.apache.geode.internal.cache.tier.sockets.command.GatewayReceiverCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.Get70;
+import org.apache.geode.internal.cache.tier.sockets.command.GetAll70;
+import org.apache.geode.internal.cache.tier.sockets.command.GetAllWithCallback;
+import
org.apache.geode.internal.cache.tier.sockets.command.GetClientPRMetadataCommand66;
+import
org.apache.geode.internal.cache.tier.sockets.command.GetClientPartitionAttributesCommand66;
+import org.apache.geode.internal.cache.tier.sockets.command.GetEntry70;
+import
org.apache.geode.internal.cache.tier.sockets.command.GetFunctionAttribute;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXEnumById;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXIdForEnum;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXIdForType;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPDXTypeById;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPdxEnums70;
+import org.apache.geode.internal.cache.tier.sockets.command.GetPdxTypes70;
+import org.apache.geode.internal.cache.tier.sockets.command.Invalid;
+import org.apache.geode.internal.cache.tier.sockets.command.Invalidate70;
+import org.apache.geode.internal.cache.tier.sockets.command.KeySet;
+import org.apache.geode.internal.cache.tier.sockets.command.MakePrimary;
+import org.apache.geode.internal.cache.tier.sockets.command.PeriodicAck;
+import org.apache.geode.internal.cache.tier.sockets.command.Ping;
+import org.apache.geode.internal.cache.tier.sockets.command.Put70;
+import org.apache.geode.internal.cache.tier.sockets.command.PutAll80;
+import org.apache.geode.internal.cache.tier.sockets.command.PutAllWithCallback;
+import org.apache.geode.internal.cache.tier.sockets.command.PutUserCredentials;
+import org.apache.geode.internal.cache.tier.sockets.command.Query651;
+import org.apache.geode.internal.cache.tier.sockets.command.QueryGeode10;
+import
org.apache.geode.internal.cache.tier.sockets.command.QueryWithParametersGeode10;
+import
org.apache.geode.internal.cache.tier.sockets.command.RegisterDataSerializers;
+import
org.apache.geode.internal.cache.tier.sockets.command.RegisterInstantiators;
+import org.apache.geode.internal.cache.tier.sockets.command.RegisterInterest61;
+import
org.apache.geode.internal.cache.tier.sockets.command.RegisterInterestList66;
+import org.apache.geode.internal.cache.tier.sockets.command.RemoveAll;
+import org.apache.geode.internal.cache.tier.sockets.command.RemoveUserAuth;
+import org.apache.geode.internal.cache.tier.sockets.command.RequestEventValue;
+import org.apache.geode.internal.cache.tier.sockets.command.RollbackCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.Size;
+import org.apache.geode.internal.cache.tier.sockets.command.TXFailoverCommand;
+import
org.apache.geode.internal.cache.tier.sockets.command.TXSynchronizationCommand;
+import org.apache.geode.internal.cache.tier.sockets.command.UnregisterInterest;
+import
org.apache.geode.internal.cache.tier.sockets.command.UnregisterInterestList;
+import
org.apache.geode.internal.cache.tier.sockets.command.UpdateClientNotification;
+import org.apache.geode.internal.serialization.KnownVersion;
+
+public class CommandInitializerTest {
+
+ @Test
+ public void testCommandMapContainsAllVersions() {
+ for (KnownVersion version : KnownVersion.getAllVersions()) {
+ if (version.isNotOlderThan(KnownVersion.OLDEST)) {
+ org.junit.Assert.assertNotNull(
+ "Please add a command set for " + version + " of Geode to
CommandInitializer",
+ CommandInitializer.getDefaultInstance().get(version));
+ }
+ }
+ }
+
+ @Test
+ public void initializeGeode18Commands() {
+ @SuppressWarnings("unchecked")
+ final Map<Integer, Command> commands = mock(Map.class);
Review comment:
I think I will keep because it is faster feedback on versioning commands
than waiting for an integration test.
##########
File path:
geode-cq/src/test/java/org/apache/geode/cache/query/cq/internal/CqServiceFactoryImplTest.java
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.cache.query.cq.internal;
+
+import static java.util.Collections.singletonMap;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
+
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.query.cq.internal.command.CloseCQ;
+import org.apache.geode.cache.query.cq.internal.command.ExecuteCQ61;
+import org.apache.geode.cache.query.cq.internal.command.GetCQStats;
+import org.apache.geode.cache.query.cq.internal.command.GetDurableCQs;
+import org.apache.geode.cache.query.cq.internal.command.MonitorCQ;
+import org.apache.geode.cache.query.cq.internal.command.StopCQ;
+import org.apache.geode.distributed.DistributedSystem;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.MessageType;
+import org.apache.geode.internal.cache.tier.sockets.CommandRegistry;
+import org.apache.geode.internal.serialization.KnownVersion;
+
+public class CqServiceFactoryImplTest {
+
+ @Test
+ public void initialize() {
Review comment:
Yes, it really doesn't make sense anymore since initialize isn't the
thing even being tested anymore. Thanks!
##########
File path:
geode-serialization/src/test/java/org/apache/geode/internal/serialization/KnownVersionJUnitTest.java
##########
@@ -44,18 +39,21 @@ public void testKnownVersionClass() throws Exception {
compare(KnownVersion.GEODE_1_10_0, KnownVersion.GEODE_1_9_0);
compare(KnownVersion.GEODE_1_11_0, KnownVersion.GEODE_1_10_0);
compare(KnownVersion.GEODE_1_12_0, KnownVersion.GEODE_1_11_0);
- compare(KnownVersion.GEODE_1_13_0, KnownVersion.GEODE_1_12_0);
- compare(KnownVersion.GEODE_1_14_0, KnownVersion.GEODE_1_13_0);
+ compare(KnownVersion.GEODE_1_12_1, KnownVersion.GEODE_1_12_0);
+ compare(KnownVersion.GEODE_1_13_0, KnownVersion.GEODE_1_12_1);
+ compare(KnownVersion.GEODE_1_13_1, KnownVersion.GEODE_1_13_0);
+ compare(KnownVersion.GEODE_1_14_0, KnownVersion.GEODE_1_13_1);
}
private void compare(KnownVersion later, KnownVersion earlier) {
assertTrue(later.compareTo(earlier) > 0);
- assertTrue(later.equals(later));
- assertTrue(later.compareTo(later) == 0);
+ assertEquals(later, later);
+ // noinspection EqualsWithItself for testing
+ assertEquals(0, later.compareTo(later));
assertTrue(earlier.compareTo(later) < 0);
assertTrue(later.compareTo(Versioning.getVersion(earlier.ordinal())) > 0);
- assertTrue(later.compareTo(Versioning.getVersion(later.ordinal())) == 0);
+ assertEquals(0, later.compareTo(Versioning.getVersion(later.ordinal())));
assertTrue(earlier.compareTo(Versioning.getVersion(later.ordinal())) < 0);
Review comment:
Static analyzer didn't pick that one up... Thanks!
----------------------------------------------------------------
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]
