dschneider-pivotal commented on a change in pull request #7375:
URL: https://github.com/apache/geode/pull/7375#discussion_r814987051
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Size.java
##########
@@ -72,7 +72,7 @@ public void cmdExecute(final @NotNull Message clientMessage,
String regionName = regionNamePart.getCachedString();
if (regionName == null) {
- logger.warn("The input region name for the %s request is null", "size");
+ logger.warn("The input region name for the size request is null");
Review comment:
while you are in this part of the code consider fixing the
errMessage.append call also. It seems like we should just have a local String
constant that is in both calls.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -5126,8 +5126,8 @@ public static void validatePRID(InternalDistributedMember
sender, int prId, Stri
PartitionedRegion pr = (PartitionedRegion) o;
if (pr.getPRId() == prId) {
if (!pr.getRegionIdentifier().equals(regionId)) {
- logger.warn("{} is using PRID {} for {} but this process maps that
PRID to {}",
- new Object[] {sender.toString(), prId,
pr.getRegionIdentifier()});
+ logger.warn("{} is using PRID {} for regionId {} but this process
maps that PRID to {}",
Review comment:
could "sender.toString()" just be "sender"? This will improve
performance if warnings are disabled (which is unlikely).
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -5613,7 +5613,7 @@ public void cleanupFailedInitialization() {
}
if (savedFirstRuntimeException != null
&& savedFirstRuntimeException instanceof
DistributedSystemDisconnectedException) {
- logger.warn("cleanupFailedInitialization originally failed with {}",
+ logger.warn("cleanupFailedInitialization originally failed with: ",
Review comment:
Did the old code for this warning log the call stack for
savedFirstRuntimeException? It looks like the intent was just to log
savedFirstRuntimeException.toString() inside the curly brackets.
But if the old behavior was to log the stack then I think your change is
good.
All the places we consider setting savedFirstRuntimeException we also log a
warning with the full stack. So I think that intent of this message was just to
say "cleanup actually did fail and here is the exception of all the ones we
already logged that caused the failure". It seems odd to me that these are
warnings. I'm not sure what a customer could do with these warnings.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
