virajjasani commented on a change in pull request #2669: URL: https://github.com/apache/hbase/pull/2669#discussion_r528106909
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ########## @@ -3485,25 +3488,26 @@ boolean checkFileSystem() { @Override public void updateRegionFavoredNodesMapping(String encodedRegionName, List<org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.ServerName> favoredNodes) { - InetSocketAddress[] addr = new InetSocketAddress[favoredNodes.size()]; + Address[] addr = new Address[favoredNodes.size()]; // Refer to the comment on the declaration of regionFavoredNodesMap on why - // it is a map of region name to InetSocketAddress[] + // it is a map of region name to Address[] for (int i = 0; i < favoredNodes.size(); i++) { - addr[i] = InetSocketAddress.createUnresolved(favoredNodes.get(i).getHostName(), + addr[i] = Address.fromParts(favoredNodes.get(i).getHostName(), favoredNodes.get(i).getPort()); } regionFavoredNodesMap.put(encodedRegionName, addr); } /** * Return the favored nodes for a region given its encoded name. Look at the - * comment around {@link #regionFavoredNodesMap} on why it is InetSocketAddress[] - * + * comment around {@link #regionFavoredNodesMap} on why we convert to InetSocketAddress[] + * here. + * @param encodedRegionName * @return array of favored locations */ @Override public InetSocketAddress[] getFavoredNodesForRegion(String encodedRegionName) { - return regionFavoredNodesMap.get(encodedRegionName); + return Address.toSocketAddress(regionFavoredNodesMap.get(encodedRegionName)); Review comment: > the place to do this would be FSUtils.java where the call to FileSystem#create including ISA parameters is performed via reflection. Oh, what a giant reflection happening here :) ``` return (FSDataOutputStream) (DistributedFileSystem.class .getDeclaredMethod("create", Path.class, FsPermission.class, boolean.class, int.class, short.class, long.class, Progressable.class, InetSocketAddress[].class) .invoke(backingFs, path, perm, true, CommonFSUtils.getDefaultBufferSize(backingFs), replication > 0 ? replication : CommonFSUtils.getDefaultReplication(backingFs, path), CommonFSUtils.getDefaultBlockSize(backingFs, path), null, favoredNodes)); ``` > We can hand the ISA directly to HDFS without checking ISA#isUnresolved because should an ISA be unresolved when HDFS tries to use it the Java network API will throw an exception, which will propagate up to us. Makes sense, I think we should be good with this as is, rather than performing resolution checks at both layers. ---------------------------------------------------------------- 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: us...@infra.apache.org