Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEODE-6796: Specially-handle UnknownHostException in jmxConnect() #3622

Merged
merged 2 commits into from Jun 11, 2019

Conversation

jackw26
Copy link
Contributor

@jackw26 jackw26 commented May 23, 2019

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?

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.

@onichols-pivotal
Copy link
Contributor

@metatype commented (on dev list) "I don’t agree that adding the exception class name creates a better user experience."

@jinmeiliao
Copy link
Member

Only in this case, it probably does offer more information if the message itself has typo's that doesn't make any sense. Another way to fix this is to check if the message itself has "nor servname" string, if so, give it another message that makes sense.

Copy link
Member

@jinmeiliao jinmeiliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think there is a better solution here, in StartLocatorCommand, you can catch the UnknownHostException specifically and provide a more sensible error message, and the leave the general exception catch block the same as before.

@onichols-pivotal
Copy link
Contributor

onichols-pivotal commented Jun 10, 2019

@jinmeiliao What qualifies as "more sensible"? If we are not going to print the exception's message, then we lose the information about exactly what hostname was not found...

Do any of these look better?

#1 (generic)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
Unable to connect.

#2 (Safari)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
Can't find the server.

#3 (golang)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
dial tcp l192.168.99.1:12345: i/o timeout

#4 (Chrome)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
Locator can't be reached. Server or IP address could not be found.

#5 (Firefox)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
The proxy server is refusing connections.

#6 (Zero Wing)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
All your server are belong to us.

#7 (2001 A Space Odyssey)
Connecting to Locator at [host=l192.168.99.1, port=12345] ..
I'm sorry, Dave. I'm afraid I can't do that.

@onichols-pivotal onichols-pivotal changed the title GEODE-6796 Now just prints the full error message. Gives user more co… GEODE-6796: Specially-handle UnknownHostException in jmxConnect() Jun 11, 2019
@onichols-pivotal
Copy link
Contributor

PR has been updated to revert the first approach and go with option #4 instead. Now the user will see:
Connecting to Locator at [host=l192.168.99.1, port=52326] ..
JMX manager can't be reached. Hostname or IP address could not be found.

@onichols-pivotal onichols-pivotal merged commit b2d08b8 into apache:develop Jun 11, 2019
jmelchio pushed a commit to jmelchio/geode that referenced this pull request Jun 13, 2019
…ache#3622)

* GEODE-6796: specially-handle UnknownHostException in jmxConnect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants