Bill commented on a change in pull request #5905:
URL: https://github.com/apache/geode/pull/5905#discussion_r561354042
##########
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:
This method is called `getKnownVersionOrDefault` in line with
`java.util.Map.getOrDefault`. It took a second parameter (like the
corresponding `Map` method does) so the caller could decide what default value
to use.
I prefer the old form of the method, even though the only use in the product
right now, sends `null` as the default. I can imagine a reasonable argument
that the only possible default value is `null`. If you want to take that tack I
recommend renaming the method. Perhaps something like `getKnownVersionOrNull`.
If the caller isn't explicitly putting `null` in the parameter list, at least
the method name suggests that they have to handle a `null` response.
##########
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:
FYI looking at the list of messages here it looks like we are missing
some. From `DSFIDFactory.create()` I see additionally:
```
case NULL_TOKEN:
return readNullToken(in);
case CONFIGURATION_RESPONSE:
return readConfigurationResponse(in);
case PR_DESTROY_ON_DATA_STORE_MESSAGE:
```
##########
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:
Why must we retain a reference to the modifiable commands? It looks like
it is to support `CqServiceFactoryImpl` calling `register()`. Why do we keep
the two separate maps?
##########
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:
is there a way to do this without calling the deprecated
`getDefaultInstance`?
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientSideHandshakeImpl.java
##########
@@ -301,18 +298,18 @@ private InternalDistributedMember
readServerMember(DataInputStream p_dis) throws
public ServerQueueStatus handshakeWithSubscriptionFeed(Socket sock, boolean
isPrimary)
throws IOException, AuthenticationRequiredException,
AuthenticationFailedException,
ServerRefusedConnectionException, ClassNotFoundException {
- ServerQueueStatus serverQueueStatus = null;
+ final ServerQueueStatus serverQueueStatus;
Review comment:
why not just return the value on line 354 and eliminate this variable
entirely?
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientSideHandshakeImpl.java
##########
@@ -336,20 +333,17 @@ public ServerQueueStatus
handshakeWithSubscriptionFeed(Socket sock, boolean isPr
if (currentClientVersion.isOlderThan(KnownVersion.GFE_61)) {
return new ServerQueueStatus(endpointType, queueSize, member);
}
- HashMap instantiatorMap = DataSerializer.readHashMap(dis);
- for (Iterator itr = instantiatorMap.entrySet().iterator();
itr.hasNext();) {
- Map.Entry instantiator = (Map.Entry) itr.next();
- Integer id = (Integer) instantiator.getKey();
- ArrayList instantiatorArguments = (ArrayList) instantiator.getValue();
- InternalInstantiator.register((String) instantiatorArguments.get(0),
- (String) instantiatorArguments.get(1), id, false);
+ final Map<Integer, List<String>> instantiatorMap =
DataSerializer.readHashMap(dis);
+ for (final Map.Entry<Integer, List<String>> entry :
instantiatorMap.entrySet()) {
Review comment:
Good cleanup here! Could go a step further:
```
instantiatorMap.entrySet().stream().forEach(
entry -> InternalInstantiator.register(entry.getValue().get(0),
entry.getValue().get(1),
entry.getKey(), false));
```
##########
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:
make `final`
##########
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:
I believe this cast is unnecessary. Same for next two casts in this
method.
##########
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:
I see what was `66` is now `11` and what was `70` is now `15`. What is
the significance of that mapping? How did you pick those replacements?
I get that you didn't want this test testing deprecated versions any more.
But why test `1_1_0`and `1_5_0` and not say `1_13_0` and `1_14_0`? Is the
choice arbitrary?
##########
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:
ok.
##########
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:
okey dokey
##########
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 map is made from the modifiable one in the constructor.
Ostensibly, any calls to `register()` after that would not result in new
entries appearing in the unmodifiable map.
I see we construct a `CommandInitializer()`. At that point the unmodifiable
map is in place. Then a ref to that object is handed to `CqService.create()`
which in turn calls `register()` on it for e.g. message type
`EXECUTECQ_MSG_TYPE`. At that point, isn't the unmodifiable map we created in
the constructor out of date since the modifiable map now contains an extra
entry?
----------------------------------------------------------------
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]
