DonalEvans commented on a change in pull request #5947:
URL: https://github.com/apache/geode/pull/5947#discussion_r562993766
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommandQuery.java
##########
@@ -434,8 +403,8 @@ private void sendResultsAsObjectArray(SelectResults
selectResults, int numberOfC
} else {
newResults = new Object[resultIndex % MAXIMUM_CHUNK_SIZE];
}
- for (int i = 0; i < newResults.length; i++) {
- newResults[i] = results[i];
+ if (newResults.length >= 0) {
Review comment:
This always evaluates to true, since arrays can't have negative length.
Should this be `if (newResults.length > 0)`?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/BaseCommand.java
##########
@@ -164,7 +166,7 @@ public void execute(Message clientMessage, ServerConnection
serverConnection,
if (EntryLogger.isEnabled() && serverConnection != null) {
Review comment:
I don't think that `serverConnection` should ever be null here, since
`execute()` is only called from one place and the value of `serverConnection`
used there cannot be null. Removing this check should fix several LGTM
warnings, while you're cleaning things up in the file.
##########
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.
Review comment:
This note seems irrelevant and possibly confusing now.
##########
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
Review comment:
Typo here. The first comma should be a space and the second comma can be
omitted.
----------------------------------------------------------------
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]
