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 2020/09/23 16:53:53 UTC

[GitHub] [geode] upthewaterspout opened a new pull request #5545: GEODE-8522: Switching a exception log back to debug

upthewaterspout opened a new pull request #5545:
URL: https://github.com/apache/geode/pull/5545


   This log message happens during the course of normal startup of multiple
   locators. We should not be logging a full stack trace during normal startup.
   
   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] upthewaterspout merged pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

upthewaterspout merged pull request #5545:
URL: https://github.com/apache/geode/pull/5545


   


----------------------------------------------------------------
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 pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

Bill commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-700116245


   Yeah I don't think `ClassNotFound` is appropriate for retry but, as is often the case, I bet the original developer used the IDE to write that catch block and since both of those exceptions are declared in the `throws` clause together, well.
   
   I think your latest change solves the problems.


----------------------------------------------------------------
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 pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

Bill commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-698610478


   @upthewaterspout so when two locators are starting up simultaneously I suppose `locatorClient.requestToServer()` is throwing `IOException` (because some other locator on `locators` list hasn't started yet.)
   
   Your comments imply that starting two locators at once is "normal". [Looking at the docs](https://geode.apache.org/docs/guide/12/configuring/running/starting_up_shutting_down.html), I suppose it is, provided you specify `locator-wait-time` (so as to avoid split-brain.) 
   
   Oddly, the docs mention:
   
   > …an info-level message
   > 
   >     GemFire startup was unable to contact a locator. Waiting for one to start. Configured locators are frodo[12345],pippin[12345].
   
   I don't find that message text anywhere in the Geode source today (searched for "unable to contact a locator" and "Configured locators are"). It makes me think that the docs might talking about (an old version) of the very message that this PR is changing (from info level down to debug level.) The logged message, on the `develop` branch and in this PR (unchanged) is:
   
   > Exception thrown when contacting a locator
   
   If that's right (that the docs are talking about the message modified by this PR), then we need a doc change to go with this code change.


----------------------------------------------------------------
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 pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

Bill commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-698654480


   @upthewaterspout would this be an improvement:
   
   ```
             logger.info("Unable to contact a locator: " + problem.getMessage());
   ```
   
   That way we pass along the content of the exception but do not print a stack trace. But we keep it at info level.
   
   Thinking a bit more: the causes and ramifications of the `IOException` seem very different from the `ClassNotFoundException` exception. I would expect the former but not the latter in your normal startup case.
   
   If we think the stack trace would be useful in the `ClassNotFoundException` case we could handle that in a separate catch block.


----------------------------------------------------------------
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] upthewaterspout commented on pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

upthewaterspout commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-699140620


   @Bill - are my new changes better? I put in an info level message.
   
   I left the catch for ClassNotFoundException alone. I'm not really clear on why that exception is appropriate for a retry in this case, but I'm hesitant to change behavior in this PR. 


----------------------------------------------------------------
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] upthewaterspout commented on pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

upthewaterspout commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-698644756

@Bill - Regarding `locator-wait-time`, `locator-wait-time` used to be something that only took effect on servers, not locators. That changed fairly recently.

It's still safe to start up multiple locators that all refer to each other without locator-wait-time - provided they can actually reach each other on startup! We have had some issues in K8s environments because there is race between when a locator tries to contact another locator and when the K8s DNS makes the locator name available.

Good catch on those docs! Yeah, I don't see that string in the products since jgroups was removed in 2015.

This particular log message *used* to be `debug` - you and I flipped it to info in this change - 53f1e1a81c3b58989a835d37f94466eb3dfc752f. I don't mind it being an info message - but I think we shouldn't be logging a stack trace in that case. Maybe just an info message that we failed to contact a particular locator. Let me know if you'd like me to make that change. Either way, I'll create a separate docs PR after we have this figured out.

----------------------------------------------------------------
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] upthewaterspout commented on pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

upthewaterspout commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-698644756

@Bill - Regarding `locator-wait-time`, `locator-wait-time` used to be something that only took effect on servers, not locators. That changed fairly recently.

Good catch on those docs! Yeah, I don't see that string in the products since jgroups was removed in 2015.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] Bill commented on pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

Bill commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-698604220


   Looks like CI failure is likely https://issues.apache.org/jira/browse/GEODE-8191


----------------------------------------------------------------
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 pull request #5545: GEODE-8522: Switching a exception log back to debug

Posted by GitBox <gi...@apache.org>.

Bill commented on pull request #5545:
URL: https://github.com/apache/geode/pull/5545#issuecomment-698604220






----------------------------------------------------------------
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