HBASE-24305 Removed deprecations in ServerName #1639
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Apache9
requested changes
May 4, 2020
| @@ -367,7 +367,7 @@ public static String parseHostFromOldLog(Path p) { | |||
| String n = p.getName(); | |||
| int idx = n.lastIndexOf(LOGNAME_SEPARATOR); | |||
| String s = URLDecoder.decode(n.substring(0, idx), "UTF8"); | |||
| return ServerName.parseHostname(s) + ":" + ServerName.parsePort(s); | |||
| return ServerName.valueOf(s).getHostname() + ":" + ServerName.valueOf(s).getPort(); | |||
There was a problem hiding this comment.
Store the valueOf result to a local variable and then call getHostname and getPort to save one parsing?
There was a problem hiding this comment.
Good point. Done.
| @@ -121,62 +117,29 @@ private ServerName(final Address address, final long startcode) { | |||
| } | |||
|
|
|||
| private ServerName(final String serverName) { | |||
| this(parseHostname(serverName), parsePort(serverName), | |||
| parseStartcode(serverName)); | |||
| this(serverName.substring(0, serverName.indexOf(SERVERNAME_SEPARATOR)), | |||
There was a problem hiding this comment.
Seems a bit awful...
Since this method private, do we really need to prevserve it? Just let the callers call other constructors?
There was a problem hiding this comment.
Refactored it to remove this private constructor completely.
| * @return True if <code>other</code> has same hostname and port. | ||
| * @param left the first server address to compare | ||
| * @param right the second server address to compare | ||
| * @return {@code true} if {@code left} and {@code right} have the same hostname and port. | ||
| */ | ||
| public static boolean isSameAddress(final ServerName left, | ||
| final ServerName right) { | ||
| // TODO: Make this left.getAddress().equals(right.getAddress()) |
There was a problem hiding this comment.
Resolve this TODO if possible?
There was a problem hiding this comment.
We use compareToIgnoreCase before but Address.equals just uses equals on host name, is it fine?
There was a problem hiding this comment.
We have a test case over in TestServerName called testHostNameCaseSensitivity, which compares a lower case version to the upper case version via isSameAddress. Does this cover your point?
Another point: Currently TestServerName is located in hbase-server, which feels a little bit strange. Should we move the test case to hbase-common in this one or should we do it separately? If we do the latter I'm also going to prepare backports, even if the original ticket is targeting 3.0.0. Probably a backport would still be good as isSameAddress was changed here and other branches would benefit from the change.
There was a problem hiding this comment.
We have a test case over in TestServerName called testHostNameCaseSensitivity, which compares a lower case version to the upper case version via isSameAddress. Does this cover your point?
Good.
And +1 on moving and backporting.
There was a problem hiding this comment.
Test class is moved to hbase-common. Also re-enabled the ignored test case within it.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
approved these changes
May 4, 2020
| @@ -367,9 +367,10 @@ public static String parseHostFromOldLog(Path p) { | |||
| String n = p.getName(); | |||
| int idx = n.lastIndexOf(LOGNAME_SEPARATOR); | |||
| String s = URLDecoder.decode(n.substring(0, idx), "UTF8"); | |||
| return ServerName.parseHostname(s) + ":" + ServerName.parsePort(s); | |||
| final ServerName serverName = ServerName.valueOf(s); | |||
| return serverName.getHostname() + ":" + serverName.getPort(); | |||
There was a problem hiding this comment.
The ServerName class comment says to do sn.getAddress() if you want to do something like what is here. What you reckon? I see you get rid of the getHostAndPort method (good). It too pointed toward sn.getAddress(). What you think?
Its more a comment for going forward. What is here is fine.
There was a problem hiding this comment.
For ServerName I would go with getHostname and getPort as it is shorter and we internally point to the address. If we want just a single way to get the information we should get rid of the shortcut methods, but I would stick with the shortcuts.
There was a problem hiding this comment.
Based on your second comment I think I got the point. We could simplify it to just use getAddress instead of constructing it ourselves. Let me try that one.
| @@ -600,7 +600,7 @@ protected ServerName getOneRandomServer(String rack, Set<ServerName> skipServerS | |||
| } | |||
|
|
|||
| if (randomServer != null) { | |||
| return ServerName.valueOf(randomServer.getHostAndPort(), randomServer.getStartcode()); | |||
| return ServerName.valueOf(randomServer.getAddress(), randomServer.getStartcode()); | |||
| String [] parts = hostname.split("\\."); | ||
| if (parts == null || parts.length == 0) return hostname; | ||
| return parts[0]; | ||
| } |
There was a problem hiding this comment.
IIRC, this was done to make it so hostnames could be logged w/o the domain name in an attempt at keeping our log lines shorter and easier to read
There was a problem hiding this comment.
Most of it is still available via toShortString, which we use most for logging.
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks for the reviews so far. Addressed the latest feedback in the latest commit. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Not sure why the build failures happen. Locally it runs fine. Let me dig into it. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
It seems like Jenkins is not taking the removal of |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Closing this in favor of #1666 as it leads to a clean build. The changes are the same. @saintstack & @Apache9 could you please check the other PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.