You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/09 18:04:00 UTC
[GitHub] [geode] bschuchardt opened a new pull request #6298: GEODE-9128: Remove host name look-up from JGAddress
bschuchardt opened a new pull request #6298:
URL: https://github.com/apache/geode/pull/6298
toString() was including the host name of the address, which is overkill
for the limited use of JGAddress.
JGroupsMessenger's logging that included this class wasn't checking the
log level before invoking logDebug() and other methods. I've added
that.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
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:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6298: GEODE-9128: Remove host name look-up from JGAddress
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6298:
URL: https://github.com/apache/geode/pull/6298#discussion_r612690648
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/messenger/JGroupsMessenger.java
##########
@@ -335,7 +335,9 @@ public void start() throws MemberStartupException {
try {
Object oldDSMembershipInfo = services.getConfig().getOldMembershipInfo();
if (oldDSMembershipInfo != null) {
- logger.debug("Reusing JGroups channel from previous system", properties);
+ if (logger.isDebugEnabled()) {
Review comment:
I've changed my mind on this. I've learned that performance tests have shown that Geode's logging infrastructure should have conditional guards around log calls. A [Design Decision](https://cwiki.apache.org/confluence/display/GEODE/Design+Decisions) on this subject is coming from @kirklund that will describe the testing and the resulting conclusion.
--
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:
users@infra.apache.org
[GitHub] [geode] bschuchardt merged pull request #6298: GEODE-9128: Remove host name look-up from JGAddress
Posted by GitBox <gi...@apache.org>.
bschuchardt merged pull request #6298:
URL: https://github.com/apache/geode/pull/6298
--
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:
users@infra.apache.org
[GitHub] [geode] Bill commented on a change in pull request #6298: GEODE-9128: Remove host name look-up from JGAddress
Posted by GitBox <gi...@apache.org>.
Bill commented on a change in pull request #6298:
URL: https://github.com/apache/geode/pull/6298#discussion_r612044863
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/messenger/JGAddress.java
##########
@@ -91,9 +91,9 @@ public String toString() {
StringBuilder sb = new StringBuilder();
if (ip_addr == null)
- sb.append("<null>");
+ sb.append("<no address>");
else {
- sb.append(ip_addr.getHostName());
+ sb.append(ip_addr);
Review comment:
yes
##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/messenger/JGroupsMessenger.java
##########
@@ -335,7 +335,9 @@ public void start() throws MemberStartupException {
try {
Object oldDSMembershipInfo = services.getConfig().getOldMembershipInfo();
if (oldDSMembershipInfo != null) {
- logger.debug("Reusing JGroups channel from previous system", properties);
+ if (logger.isDebugEnabled()) {
Review comment:
This extra conditional (and the others like it) don't seem to improve performance or maintainability. Is there a coding standard for Geode, that necessitates this pre-checking?
I searched the codebase for e.g. "logger.debug(" calls and while many do the pre-check like this, many others do not.
--
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:
users@infra.apache.org