Bill commented on a change in pull request #5947:
URL: https://github.com/apache/geode/pull/5947#discussion_r564141760
##########
File path:
geode-core/src/main/java/org/apache/geode/cache/client/internal/ClientSideHandshakeImpl.java
##########
@@ -237,17 +229,12 @@ public ServerQueueStatus handshakeWithServer(Connection
conn, ServerLocation loc
readMessage(dis, dos, acceptanceCode, member);
// Read delta-propagation property value from server.
- // [sumedh] Static variable below? Client can connect to different
- // DSes with different values of this. It shoule be a member variable.
- if (!communicationMode.isWAN() &&
currentClientVersion.isNotOlderThan(KnownVersion.GFE_61)) {
+ if (!communicationMode.isWAN()) {
((InternalDistributedSystem)
system).setDeltaEnabledOnServer(dis.readBoolean());
}
// validate that the remote side has a different distributed system id.
- if (communicationMode.isWAN()
- && KnownVersion.GFE_66
- .compareTo(Versioning.getVersion(conn.getWanSiteVersion())) <= 0
- && currentClientVersion.isNotOlderThan(KnownVersion.GFE_66)) {
+ if (communicationMode.isWAN()) {
Review comment:
it's ok to remove the two clauses without further ado, since they
mention GFE_66 (support for which goes away with this PR) ✓
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskInitFileParser.java
##########
@@ -65,14 +65,10 @@ public DiskInitFileParser(CountingDataInputStream dis,
DiskInitFileInterpreter i
private transient boolean gotEOF;
public DiskStoreID parse() throws IOException, ClassNotFoundException {
- KnownVersion gfversion = KnownVersion.GFE_662;
+ KnownVersion gfversion = KnownVersion.GFE_70;
DiskStoreID result = null;
boolean endOfFile = false;
- while (!endOfFile) {
- if (dis.atEndOfFile()) {
- endOfFile = true;
- break;
- }
+ while (!(endOfFile || dis.atEndOfFile())) {
Review comment:
ok to eliminate assignment since we never use `endOfFile` outside this
(`while`) block ✓
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
##########
@@ -606,38 +605,37 @@ boolean processHandShake() {
return result;
}
} finally {
- if (isTerminated() || !result) {
- return false;
- }
- boolean registerClient = false;
- synchronized (getCleanupProxyIdTable()) {
- MutableInt numRefs = getCleanupProxyIdTable().get(proxyId);
- if (numRefs != null) {
- numRefs.increment();
- } else {
- registerClient = true;
- getCleanupProxyIdTable().put(proxyId, new MutableInt(1));
+ if (!isTerminated() && result) {
Review comment:
Without any analysis of `isTerminated()` it seems to me that eliminating
the `return false;` statement changes the behavior of this method. It's a
pretty significant change potentially since returning from inside a `finally`
causes any exceptions that may have gotten us here, to be discarded (ignored.)
To keep the old behavior, you could add an `else { return false; }`
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/SocketMessageWriter.java
##########
@@ -45,46 +47,41 @@ public void writeHandshakeMessage(DataOutputStream dos,
byte type, String p_msg,
msg = "";
}
dos.writeUTF(msg);
- if (clientVersion != null &&
clientVersion.isNotOlderThan(KnownVersion.GFE_61)) {
- // get all the instantiators.
- Instantiator[] instantiators = InternalInstantiator.getInstantiators();
- HashMap instantiatorMap = new HashMap();
- if (instantiators != null && instantiators.length > 0) {
- for (Instantiator instantiator : instantiators) {
- ArrayList instantiatorAttributes = new ArrayList();
-
instantiatorAttributes.add(instantiator.getClass().toString().substring(6));
-
instantiatorAttributes.add(instantiator.getInstantiatedClass().toString().substring(6));
- instantiatorMap.put(instantiator.getId(), instantiatorAttributes);
- }
+
Review comment:
It appears to me (from looking at `ClientCacheNotifier`) that
`clientVersion` is _probably_ never `null`. Are you confident that the null
check was superfluous?
It's _almost_ not used anymore. But it is still used.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -6395,20 +6395,12 @@ private Object deserializeKey(byte[] keyBytes, final
KnownVersion version,
*/
public KnownVersion getProductVersionIfOld() {
final KnownVersion version = this.gfversion;
- if (version == null) {
- // check for the case of diskstore upgrade from 6.6 to >= 7.0
- if (getParent().isUpgradeVersionOnly()) {
- // assume previous release version
- return KnownVersion.GFE_66;
- } else {
- return null;
- }
- } else if (version == KnownVersion.CURRENT) {
+ if (version == KnownVersion.CURRENT) {
return null;
- } else {
- // version changed so return that for VersionedDataStream
- return version;
}
+
+ // version changed so return that for VersionedDataStream
+ return version;
Review comment:
ok that eliminated some tortured logic ✓
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
##########
@@ -1699,7 +1696,7 @@ private UserAuthAttributes getUserAuthAttributes() throws
IOException {
if (isTerminated()) {
throw new IOException("Server connection is terminated.");
}
- logger.debug("Unexpected exception {}", npe);
+ logger.debug("Unexpected exception {}", npe.toString());
Review comment:
Ah yes you fixed a bug here!
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientHealthMonitor.java
##########
@@ -665,16 +663,8 @@ private ServerConnectionCollection
getProxyIdCollection(ClientProxyMembershipID
return cleanupTable;
}
- private int getNumberOfClientsAtOrAboveVersion(KnownVersion version) {
- int number = 0;
- for (int i = version.ordinal(); i < numOfClientsPerVersion.length(); i++) {
- number += numOfClientsPerVersion.get(i);
- }
- return number;
- }
-
public boolean hasDeltaClients() {
- return getNumberOfClientsAtOrAboveVersion(KnownVersion.GFE_61) > 0;
+ return numberOfClientsWithConflationOff.get() > 0;
Review comment:
Verified that `ClientCacheNotifier` is indeed keeping track of only
those clients that have conflation _off_ ✓ Demeter would not be happy but he is
beyond the scope of this PR.
##########
File path:
geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
##########
@@ -1713,15 +1713,15 @@ toData,17
org/apache/geode/internal/cache/tier/sockets/ClientDataSerializerMessage,2
fromData,82
-toData,85
+toData,99
org/apache/geode/internal/cache/tier/sockets/ClientDenylistProcessor$ClientDenylistMessage,2
fromData,25
toData,25
org/apache/geode/internal/cache/tier/sockets/ClientInstantiatorMessage,2
fromData,82
-toData,85
+toData,99
Review comment:
I went over to the message classes and verified that the toData/fromData
changes over there do not change on-the-wire data ✓
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommand.java
##########
@@ -1347,14 +1304,10 @@ private static void handleRegEx(LocalRegion region,
String regex, InterestResult
*/
private static void handleRegExPR(final PartitionedRegion region, final
String regex,
final InterestResultPolicy policy, final ServerConnection servConn)
throws IOException {
- final List keyList = new ArrayList(MAXIMUM_CHUNK_SIZE);
+ final List<Object> keyList = new ArrayList<>(MAXIMUM_CHUNK_SIZE);
region.getKeysWithRegEx(regex, sendTombstonesInRIResults(servConn, policy),
- new PartitionedRegion.SetCollector() {
- @Override
- public void receiveSet(Set theSet) throws IOException {
- appendInterestResponseKeys(region, regex, theSet, keyList,
servConn);
- }
- });
+ theSet -> appendInterestResponseKeys(region, regex,
uncheckedCast(theSet), keyList,
Review comment:
I think the answer is "nothing". OTOH this looks like a trivial
IntelliJ-suggested replace-with-lambda.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientHealthMonitor.java
##########
@@ -128,22 +127,21 @@ public int getMaximumTimeBetweenPings() {
new HashMap<>();
/**
- * Gives, version-wise, the number of clients connected to the cache servers
in this cache, which
+ * Gives,the number of clients connected to the cache servers in this cache,
which
* are capable of processing received deltas.
*
* NOTE: It does not necessarily give the actual number of clients per
version connected to the
* cache servers in this cache.
*
* @see CacheClientNotifier#addClientProxy(CacheClientProxy)
*/
- AtomicIntegerArray numOfClientsPerVersion =
- new AtomicIntegerArray(KnownVersion.HIGHEST_VERSION + 1);
+ AtomicInteger numberOfClientsWithConflationOff = new AtomicInteger(0);
Review comment:
Nice simplification here! Appreciate the meaningful name!
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Get70.java
##########
@@ -109,12 +104,13 @@ public void cmdExecute(final Message clientMessage, final
ServerConnection serve
// Process the get request
if (key == null || regionName == null) {
+ final String errMessage;
if ((key == null) && (regionName == null)) {
errMessage =
"The input region name and key for the get request are null.";
} else if (key == null) {
errMessage = "The input key for the get request is null.";
- } else if (regionName == null) {
+ } else {
Review comment:
`regionName == null` is always `true` ✓
----------------------------------------------------------------
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]
